Updated Metadata property on connection to be string map #160
Conversation
Metadata property on connection should be the same data type as "client_metadata" would be on an application/client. Changed it to map[string]string instead of *interface{}
There was a problem hiding this comment.
Hey @robotsrule thanks for the contribution! This looks good, I've left a few comments but nothing too major, let me know what you think :)
| @@ -70,7 +70,7 @@ type Connection struct { | |||
| // connection name will be added as realm. | |||
| Realms []interface{} `json:"realms,omitempty"` | |||
|
|
|||
| Metadata *interface{} `json:"metadata,omitempty"` | |||
| Metadata map[string]string `json:"metadata,omitempty"` | |||
There was a problem hiding this comment.
I think this should be map[string]interface{}, to allow values other than strings. The SDK does similar in other locations.
There was a problem hiding this comment.
@paddycarey thanks for the feedback.
I just experimentally verified against Auth0's management API that it will only accept string values. I tried setting objects and numbers and it failed validation and returned a 400 in each case.
Auth0 hasn't done a good job of documenting this particular field but as far as I can determine it will only allow string values.
It is also worth noting that the data type for ClientMetadata on the client resource is also defined as map[string]string in this SDK and not map[string]interface{}
So I think map[string]string is probably correct in this specific case
management/connection_test.go
Outdated
| expect.Expect(t, g.Metadata["foo"], "bar") | ||
| expect.Expect(t, g.Metadata["bat"], "baz") |
There was a problem hiding this comment.
I'm not sure this test adds much value here, since it's just checking that the values set initially are still set. If we really wanted to check that setting metadata worked we should set it, then retrieve a new instance via a GET request to check against.
I'm not sure it's necessary though, since we're not doing the same for other fields, only on the Options field currently.
There was a problem hiding this comment.
I think you are right, I should have looked more closely at what this test is actually doing. I will remove it.
The ClientMetadata field on the client resource is similarly untested and that is the most similar thing in the SDK to the connection metadata so it is probably fine.
|
This closes #179 |
First off, I have no experience in GoLang so I'm not sure this is the correct solution to this problem. Please let me know if anything needs to be changed with this PR.
This is step one of me attempting to fix an issue with the terraform provider that utilizes this library.
Issue 282 on alexkappa/terraform-provider-auth0
When attempting to fix that in the terraform provider I noticed that the data type for the property was an interface pointer which doesn't make sense to me.
I think the Metadata property on connection should be the same data type as "client_metadata" would be on an application/client since connection metadata functions in exactly the same way as client metadata. I changed it to map[string]string instead of *interface{} and modified a test case to set the property and expect that it was set.