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

name field clashes with top-level envelope of all responses #66

Open
mgrbyte opened this issue Feb 4, 2017 · 12 comments
Open

name field clashes with top-level envelope of all responses #66

mgrbyte opened this issue Feb 4, 2017 · 12 comments
Assignees
Projects

Comments

@mgrbyte
Copy link
Contributor

mgrbyte commented Feb 4, 2017

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.

@mgrbyte
Copy link
Contributor Author

mgrbyte commented Feb 4, 2017

Copied over from a comment in #61

@sibyl229
Copy link
Contributor

sibyl229 commented Feb 6, 2017

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.

@mgrbyte
Copy link
Contributor Author

mgrbyte commented Feb 6, 2017

@sibyl229 Are all widget fields currently published by the (perl) catalyst rest api?
If so, how does it deal with this case (if at all?)
If it's not breaking bw compat, my suggestion would be to change the top-level envelope for all field apis to
json {"name": ..., "id": ...., "class": ... , "field": {"<FIELD_NAME>":: ... }}

@sibyl229
Copy link
Contributor

sibyl229 commented Feb 6, 2017

@mgrbyte the catalyst rest api publishes all the field level endpoints. It doesn't deal with this case though.
I agree that the structure you suggested is most sensible. In terms of being backward compatible, the website code can be fixed easily. The uncertain impact is on external user who might rely on the current data structure. Personally, I think very few users will be affected, and their code can be fixed relatively easily. But I can't really make the call. @tharris is away this week, I can bring this up with him when he is back next week. What do you think?

@mgrbyte
Copy link
Contributor Author

mgrbyte commented Feb 6, 2017

@sibyl229 I think that's fine.
Perhaps it would be worth a look at the (google?) analytics, see if it's possible to determine how many users hit the /name endpoint?

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:
A ?version query parameter (or path prefix /v1 , /v2) added to (all) rest API URLs.

This is done, I think, to permit changing rest API endpoints between versions, but better than changing whAPIs
(leave the current unversioned API URLs pointing at v1).

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 /rest/field/ responses to contain their data under a "field" key as I propose above.

@mgrbyte mgrbyte closed this as completed Feb 6, 2017
@mgrbyte
Copy link
Contributor Author

mgrbyte commented Feb 6, 2017

@sibyl229 I've thought a little on this, here's 2 x solutions...

1: Versioned apis

In 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 (?version=v1) or path-param (/v1/rest/...).

In order to break any external consumers, we'd need to default the API w/out a version identifier to v1,
but make the website use v2. As it stands, we'd also need to effectively publish two applications.
The whole thing becomes quite messy, but it's doable.

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.

  • Publish a new endpoint for each widget called /entity-name.
  • Make all widgets in d-t-c, and in any referring website-code use entity-name instead of name (in widgets as well as standalone fields)
  • Leave an independant field in place the with current behaviour.

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 "field" as I suggested in previous comment(s) above)

  • See Rich Hickey's keynote talk given a clojureconj 2016 "Spec-ulation", and other reading in this area

@sibyl229
Copy link
Contributor

sibyl229 commented Feb 7, 2017

@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 name to entity-name. This way, only rest/field/ level endpoints signature will be affected, not rest/widget/ ones. What do you think?

@mgrbyte
Copy link
Contributor Author

mgrbyte commented Feb 8, 2017

@sibyl229 your "hacky" suggestion works just fine (and indeed is the only way implementing 2 really!) 👍

@mgrbyte mgrbyte reopened this Feb 8, 2017
@mgrbyte
Copy link
Contributor Author

mgrbyte commented Feb 8, 2017

Re-opened this - assume we want to implement it? @sibyl229 @a8wright

@mgrbyte mgrbyte added this to Pending in Gene APIs Feb 8, 2017
@mgrbyte
Copy link
Contributor Author

mgrbyte commented Aug 8, 2017

@sibyl229 Just pinging on this to see if it should be closed or if it still needs doing?

@sibyl229
Copy link
Contributor

sibyl229 commented Aug 8, 2017

Hi @mgrbyte right, this still needs doing. I forgot about it >.<

@mgrbyte
Copy link
Contributor Author

mgrbyte commented Aug 9, 2017

@sibyl229 no worries, just making sure I didn't close preemptively 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Gene APIs
Pending
Development

No branches or pull requests

3 participants