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

Expand API documentation for nfs_export_add #63

Open
tasleson opened this issue Aug 11, 2020 · 6 comments · May be fixed by #72
Open

Expand API documentation for nfs_export_add #63

tasleson opened this issue Aug 11, 2020 · 6 comments · May be fixed by #72
Assignees

Comments

@tasleson
Copy link
Member

PR #60 added return value to the API call nfs_export_add. Update documentation to reflect.

Also, before we cut the next release we should scrutinize this call and see if there is anything else we want to add to returned data.

@tasleson
Copy link
Member Author

Related: #62 😹

@dsonck92
Copy link
Contributor

As for adding, considering that (probably) most will ignore extraneous fields, that might not be too important, however a consistent naming would be a must. It's much harder if an API suddenly changes the name which breaks clients expecting specific names (which I think is safe to assume nearly all do).

Train of thought I do not endorse:
Technically if the return value is guaranteed to be a dict that may change but upon giving to a related function will perform the correct action, might allow clients to treat it as dynamic data. This would still allow UI's to show some feedback but obviously would limit any meaningfulness of the contained data. To visualize:

deletion_data = jsonrequest('nfs_export_add', options)
# Time passes
jsonrequest('nfs_export_delete', **deletion_data)

@dsonck92
Copy link
Contributor

Just as a note, I can pick this up if it's not done already (I don't believe it is but I might have missed stuff)

@tasleson
Copy link
Member Author

Just as a note, I can pick this up if it's not done already (I don't believe it is but I might have missed stuff)

I've not done any work on this. I'll try to assign issues to myself before I work on them to avoid duplication of effort.

@tasleson
Copy link
Member Author

tasleson commented Sep 3, 2020

@dsonck92 I think your approach of returning a dictionary is good. It should provide backwards compatibility while allowing us to add stuff moving forward.

@dsonck92
Copy link
Contributor

dsonck92 commented Sep 3, 2020

Meanwhile, at work, I did find one possible library that could be failing, Jackson. If someone decides to implement a client for Java and uses Jackson as serializer/deserializer for the json objects, by default, it will throw on unknown properties. That said, this can be worked around for them in two ways:

  • Let Jackson ignore unknown fields
  • Just use Map<String,String>, aka, just store all fields that you receive as a dict

In the end, it boils down to documentation. I think, if we specify that it's possible fields will be added to the response, people that might use libraries which follow the Jackson approach of erroring when it's not exactly like you tell the library it is, can prepare for the issues. Either by asking their library of choice to ignore them, add them unconditionally in case new useful return values get added or purposely enabling it and do some integration testing so they sit on top of compatibility. Considering that, if the documentation always gets updated in tandem of api changes, it could even be done by them through a notification of the documentation file changing.

Anyways, I will see if I can add it this weekend, probably Sunday.

@dsonck92 dsonck92 linked a pull request Sep 13, 2020 that will close this issue
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

Successfully merging a pull request may close this issue.

2 participants