-
Notifications
You must be signed in to change notification settings - Fork 2
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
name field clashes with top-level envelope of all responses #66
Comments
Copied over from a comment in #61 |
Hi @mgrbyte Good finding this! A solution seem tricky. Nothing is really ideal. I'm okay with make it a special case, either not publishing the name field, or, alternatively, mapping name field to a different key. I think the fields in general should stay published to minimize impact on users relying on these field level APIs. |
@sibyl229 Are all widget fields currently published by the (perl) catalyst rest api? |
@mgrbyte the catalyst rest api publishes all the field level endpoints. It doesn't deal with this case though. |
@sibyl229 I think that's fine. I've not got a good picture of the external usage of the API, but if it's a low number of people using all the /field/ endpoints, it may be worth doing. In the wild, a solution a see to this kind of thing is API URL versioning: This is done, I think, to permit changing rest API endpoints between versions, but better than changing whAPIs Perhaps then document and publish the versions on the website API tools page, and list the difference(s) (Which will be mainly the change to the "envelope" of |
@sibyl229 I've thought a little on this, here's 2 x solutions... 1: Versioned apisIn the wild, the solution to (rest) API change I've seen most commonly involves versioning APIs by including a version identifier in the URL (query-param ( In order to break any external consumers, we'd need to default the API w/out a version identifier to v1, 2: Add new endpoint with new name, keep old one.Add a new endpoint with a name that doesn't trample on the "name" field in the outer response envelope.
This would require updating the website code to refer to "entity-name" rather than "name" in (wb-dev) consumers of both field and widget endpoints, but would not affect external consumers. We can even add a "deprecated" meta-data to the existing "name" endpoint to the swagger UI. Of the two solutions above, I'd prefer the later - on the grounds it's less work for us and less work for any external user(s) who may be effected. I think following the recommendation* of clojure's BDFL of never changing the behaviour attached to a (public) name is also good advice for rest APIs. (N.B: solution 2 would mean not changing the envelope to include
|
@mgrbyte thoughtful analysis! I also prefer the latter approach. I have a somewhat hacky suggestion, not sure if it's appropriate, but it will save us some work. So basically, within the route creator for fields (in routing.clj), rename every field called |
@sibyl229 your "hacky" suggestion works just fine (and indeed is the only way implementing 2 really!) 👍 |
Re-opened this - assume we want to implement it? @sibyl229 @a8wright |
@sibyl229 Just pinging on this to see if it should be closed or if it still needs doing? |
Hi @mgrbyte right, this still needs doing. I forgot about it >.< |
@sibyl229 no worries, just making sure I didn't close preemptively 👍 |
Currently, the routing code publishes all (gene) widget fields under the URI prefix /rest/field/gene/ -
and each response contains:
{:name "gene" :uri "rest/field/gene/<id>/<field-name>" :<field-name> <data>}
This is an issue when
<field-name>
is the name field, since it overrides the name field at the top-level which is intended to be "gene".We can either make a special case in the code for not publishing the name field, turn off publication of all gene widget fields by default or something else.
The text was updated successfully, but these errors were encountered: