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

Restrict Resource output to string-keyed collections #539

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bennostein
Copy link
Contributor

The default REST server implementation passes HTTP path parameters straight through to Skip, which caused me a bit of confusion when resources have non-string keys.

We may avoid putting key parameters straight into the path in the future, but for now this prevents the confusion I ran into (i.e. requesting a path like /users/1 and getting nothing because the user is keyed on the number 1 and not the string "1")

See SkipLabs#517: The default REST server implementation passes path-params straight
through to Skip, which causes some confusion when resources have non-string
keys.
skipruntime-ts/tests/src/tests.ts Outdated Show resolved Hide resolved
skipruntime-ts/examples/departures.ts Outdated Show resolved Hide resolved
@jberdine
Copy link
Contributor

Question: would changing from GET to POST a la #535, which would put the resources and params into the request body as json instead of being encoded in the URL, have avoided the original confusion?

@bennostein
Copy link
Contributor Author

Question: would changing from GET to POST a la #535, which would put the resources and params into the request body as json instead of being encoded in the URL, have avoided the original confusion?

As I understand it, yes! Lucas mentioned (offline) wanting to do that at some point too, and in particular emphasizing that a sync read instantiates a resource, which could be misunderstood under the current scheme where the key is a path parameter.

@jberdine
Copy link
Contributor

Question: would changing from GET to POST a la #535, which would put the resources and params into the request body as json instead of being encoded in the URL, have avoided the original confusion?

As I understand it, yes! Lucas mentioned (offline) wanting to do that at some point too, and in particular emphasizing that a sync read instantiates a resource, which could be misunderstood under the current scheme where the key is a path parameter.

Ok good. This makes me wonder if we should make the POST change instead of reducing the generality of the key type.

@bennostein
Copy link
Contributor Author

Seems reasonable to me. I'll table this for the time being, but still going to keep working on docs-writing, not implement that POST change.

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 this pull request may close these issues.

3 participants