Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create Automation Schedule, Interval field type problem #725

Closed
Kemyke opened this issue Aug 15, 2017 · 5 comments
Closed

Create Automation Schedule, Interval field type problem #725

Kemyke opened this issue Aug 15, 2017 · 5 comments
Assignees

Comments

@Kemyke
Copy link

Kemyke commented Aug 15, 2017

In the type ScheduleCreateOrUpdateProperties there is an Interval field. It's type is *map[string]interface{}. I couldn't find any documentation what kind of map should i use there. I checked the powershell and the c# SDKs and they use a simple byte field there.

If this is the proper type of Interval can you someone provide some information what keys and values should i add to that dictionary?

@marstr marstr self-assigned this Aug 15, 2017
@marstr
Copy link
Member

marstr commented Aug 15, 2017

Howdy @Kemyke,

The type *map[string]interface{} is coming from our generator's mapping of OpenAPI types to Go Types. In particular, we only use map[string]interface{} when we don't have enough information about the service to be more specific. Digging into the particular operation that you're mentioning, you can see that Interval doesn't specify any type information.

Now, that doesn't mean that the situation is hopeless! I'll open an issue in the azure-specs-repository asking to improve the definition of the Interval field. In the mean time, to unblock you, I tinkered around with Go and didn't find a good way to json.Serialize a map[string]interface{} into a JSON token instead of a JSON object the way the C# SDK would. The next step, assuming we can't find a way to structure the map, would be to create a custom Preparer which will set the value of Interval on the response manually. I recognize that it's a lot of work for little return, but it may be the fastest way to get unblocked while we fix it the right way.

@marstr
Copy link
Member

marstr commented Aug 15, 2017

Because there aren't any more actionable items in this issue, I'm going to close it out. However, feel free to keep commenting here and ping me if needed.

@marstr marstr closed this as completed Aug 15, 2017
@Kemyke
Copy link
Author

Kemyke commented Aug 16, 2017

I know it is dirty but isn't it a possible workaround to modify the models.go and change the type of the Interval field manually on this type until the rest api spec fixed?

@Kemyke
Copy link
Author

Kemyke commented Aug 16, 2017

And one more thing, may i ask why you use map[string]interface{} for the unkown types and not an empty interface (interface{})

@marstr
Copy link
Member

marstr commented Aug 16, 2017

You're free to do that in a vendored copy of our SDK, and that may be easier for you than building a preparer. Do note though, that beyond just feeling dirty, the consequence of doing so is that updating your dependency will wipe away your work.

In terms of the choice of map[string]interface{}, this is something that puzzles me too. I've dived into it before, and convinced myself that interface{} was a poor choice. If I remember correctly, I basically think that the type assertion story in the default json.Unmarshal & json.Marshal functions isn't strong enough to allow for just interface{}. (This is a bit of a cop-out statement, I know please forgive me.) If I were going to choose anything, I would have thought json.RawMessage would be a reasonably good choice.

@jhendrixMSFT, @mcardosos, @salameer have any thoughts on this? I think it probably merits someone spending a little time next sprint evaluating whether or not switching to json.RawMessage would be safe.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants