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

It should be forbidden to invent own path variable names #128

Closed
christophfriedrich opened this issue Sep 30, 2018 · 5 comments
Closed

It should be forbidden to invent own path variable names #128

christophfriedrich opened this issue Sep 30, 2018 · 5 comments
Milestone

Comments

@christophfriedrich
Copy link
Contributor

I'm currently building a "method name -> endpoint" map for the JS client's implementation of hasFeature (background: #125) and realised that it'll be very important that all back-ends adhere to the same syntax and names when paths incorporate variables. E.g. I hard-coded describeJob to belong to GET /jobs/{job_id} - but if some back-end outputs /jobs/{$jobId} as the path in its Capabilities, the check would result in a false-negative.

The API does already say:

Variables in the paths must be placed in curly braces, e.g. {process_id}, and have a self-speaking name. It is RECOMMENDED that the path variable names fully follow the API specification.

Measured by these rules, my "false-negative" example above would be completely valid. But it can make a lot of problems.

I don't see any reason why we should leave the possibility of inventing own names by only "recommending" that all names follow the spec. Demanding "self-speaking names" kind of cries "it's okay if you make up your own, just make sure that they're understandable". I don't see any use case for this. Only problems.

Therefore I would like to make that spec paragraph stricter by changing it to:

Variables in the paths MUST be placed in curly braces, e.g. {process_id}, and MUST NOT be pre- or postfixed (e.g. NO {$process_id}). Path variable names MUST fully follow the API specification, i.e. they MUST be part of a path given in the spec*. The spec shall only introduce self-speaking names, and these shall consist of lower-case alphanumeric characters + underscores only (i.e. "snake case").

*) I didn't know how to word this part, basically I want to achieve:

They MUST be part of this enum: "user_id" "job_id" .... If it's not in there, it's not allowed.

@m-mohr
Copy link
Member

m-mohr commented Oct 4, 2018

Have you checked the old implementation? This was already fail-safe in the old JS client.

@christophfriedrich
Copy link
Contributor Author

I'm talking about what the API spec permits, not about the implementation of the JS client, so I'm a bit confused what you mean 🤔 I linked to the doc I read (the blue "say" above).

@m-mohr
Copy link
Member

m-mohr commented Oct 8, 2018

I'm neutral on this issue. We can require it, but the current state can also easily be implemented with regular expression. So maybe let's put this on hold and wait how strict the back-ends follow this standard and reconsider changing it just before a 0.4 release.

@christophfriedrich
Copy link
Contributor Author

I'd prefer to force the back-ends to follow the spec, because that's what a spec is for, but I can deal with your suggestion.

@m-mohr m-mohr added this to the v1.0 milestone Nov 18, 2018
m-mohr added a commit that referenced this issue Jan 9, 2019
@m-mohr m-mohr modified the milestones: v1.0, v0.4 Jan 9, 2019
@m-mohr
Copy link
Member

m-mohr commented Jan 9, 2019

Fixed for API v0.4.

@m-mohr m-mohr closed this as completed Jan 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants