Validate whether user input passes API constraints?

christophfriedrich commented 5 years ago

For example, a collection name must pass the regex ^[A-Za-z0-9_\-\.~\/]+$, so I implemented a regex in the describeCollection method that checks whether the user-supplied name parameter adheres to it.

Pro:

user input is validated (obviously)

Con:

if the API changes (e.g. ~ is removed from allowed characters) the client needs updating too, and this kind of change is hard to spot/may be forgotten easily
regex test may be slow, especially if querying multiple datasets at the same time (however the regex is probably not going to be the bottleneck compared to network and database lookup speeds)
errors are quite unlikely to ever happen since names will most certainly always be taken from the list supplied by /collections and therefore be valid anyways (i.e.: it's not really user input)
if errors do happen, the back-end is gonna tell the user anyway that the name was incorrect
or, if a name is taken from /collections and passed into describeCollection and fails the test, it may be invalid according to the spec -- but nevertheless the back-end is using it and our lib would be in the way of making that work. (Ok, when it's invalid it's there own fault, but still.)

Therefore I'm considering to remove that regex.

m-mohr commented 5 years ago

I don't see many of the cons as cons - regexp speed is not a problem for example - but overall I'm neutral.

mkadunc commented 5 years ago

I agree with removing this, primarily because of SSOT and 4th bullet in the Con section above.

Validation in the client API only makes sense in so far as it produces a different result than the alternative (which is validation on the server-side) - eagerly checking parameters in a function only to have them checked again further down the stack is just extra code for no apparent gain.

If the regex-checking code was auto-generated from the API schema, it might make sense to have this on the client, until then I suggest to go with the least possible code duplication.

christophfriedrich commented 5 years ago

Thanks for your feedback Miha, that was the last impulse to make me remove it. This issue will auto-close once that commit is merged into the main repo's master branch.

christophfriedrich commented 5 years ago

Meanwhile that commit has been merged into the development branch here.

Open-EO / openeo-js-client

Validate whether user input passes API constraints? #8