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

Handling of new (previously None) return values #62

Open
dsonck92 opened this issue Aug 11, 2020 · 5 comments
Open

Handling of new (previously None) return values #62

dsonck92 opened this issue Aug 11, 2020 · 5 comments

Comments

@dsonck92
Copy link
Contributor

As was remarked in #60 (and #59) the API does not utilize return values much and it would be useful to discuss how to change this.

My current interpretation is:

  • Most clients that don't expect a return value will ignore them
    • This holds at least for go when the "return pointer" is nil

This issue should be used for discussing how it would be best to deal with it.
Note: the scope of this is limited to API endpoints that previously return None or for values added in addition to the old return values. Any API already returning values that now get a renamed return should be handled differently and is outside of this scope.

@tasleson
Copy link
Member

My current thinking is OK to add returned value that doesn't have one, but once we add for an API call they are cast in stone. If we need additional values returned later we would then introduce a new API call. Does this sound right?

I'm going to code up a simple golang client to see if this holds true or if we have some wiggle room. For languages that treat JSON struct as hash it makes it easier to update, but not all languages do that.

@dsonck92
Copy link
Contributor Author

Well, I'm actually expecting (considering people using json.Unmarshal for decoding the returned value) that adding fields does not break golang clients. However removal and renames will definitely break it. Although, then again, it might set them to nil. I'll verify with the manual

@dsonck92
Copy link
Contributor Author

Well, what better than using the playground: https://play.golang.org/p/GSfUrNdxD3t

This answered two questions for me:

  • Golang will not error out on missing fields (though any code afterwards in the client might be illegal)
  • Golang will ignore the extraneous fields, at least by default

There are further options that can be set if the client wants to by using Decoder which also has a DisallowUnknownFields which would definitely error out on extra fields. But personally I think most will use Unmarshal directly. Mostly because, in order for a jsonrpc client to be universal, it's easiest to just let go decode it into the users struct.

@triluch
Copy link
Contributor

triluch commented Aug 11, 2020

I see a lot has been going on about API, new values etc.

For starters:

One of my pet peeves is when people break API's, thus my insistence on maintaining API compatibility.
That's my opinion too. However, as usual, there are two sides of this: maintaining compatibility and adding new features.

Adding return value to call that hadn't one should be OK, but it will probably make sense to check behavior of major clients when this changes (libStorageManagement - that one is done in current tests).

There is slight possibility that some clients may break as they are mapping json response to object - if they now expect null/None in "result" fileld, but I think thats unlikely. There are not that many clients using targetd.

As for adding/changing return values or method calls in the future - maybe we should think about versioning API. In json-rpc it is possible to use dot as separator, so that may be leveraged.

For example, all current calls should be able to be made as they are now, but if API changes we could add prefix, like: "v2.method" and add additional mapping to actual functions in the code based also on this version parameter (and if there is no specific mapping for API "v2" and "method" just use default one).

That's basically same as @tasleson said:

If we need additional values returned later we would then introduce a new API call.
Just a bit different approach.

@dsonck92
Copy link
Contributor Author

I was also thinking at versioning the methods, not knowing the ability noted by @triluch. It seems like we share the same view of prefixing the API when a breaking change happens.

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

No branches or pull requests

3 participants