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

fix: Allow OPTIONS requests to RPC #1839

Closed
wants to merge 5 commits into from

Conversation

laurenceisla
Copy link
Member

This PR handles OPTIONS requests to functions, mentioned here: #1826

Won't add the 405 response to a GET for a volatile function feature due to the routing refactor as discussed here

@laurenceisla laurenceisla marked this pull request as draft April 29, 2021 00:27
@steve-chavez steve-chavez changed the title Rpcoptions fix: RPC OPTIONS Apr 29, 2021
@laurenceisla laurenceisla changed the title fix: RPC OPTIONS fix:Allow OPTIONS requests to RPC Apr 29, 2021
@steve-chavez steve-chavez changed the title fix:Allow OPTIONS requests to RPC fix: Allow OPTIONS requests to RPC Apr 29, 2021
@laurenceisla laurenceisla marked this pull request as ready for review April 29, 2021 00:39
@wolfgangwalther
Copy link
Member

Right now you're returning GET for a /rpc/some_overloaded_functions endpoint, when any of the overloaded functions are non-volatile. I don't think that's correct in the HTTP/REST context.

URIs in HTTP/REST should be completely replaceable - i.e. /rpc/some_overloaded_functions and /rpc/some_overloaded_functions?arg=1&arg=2 are completely independent of each other. You should not need to make assumptions about the URI with arguments from the OPTIONS request to the URI without arguments.

Right now, an OPTIONS /rpc/options_test_overloaded_at_least_one_non_volatile will return OPTIONS,GET,HEAD,POST - but that's wrong. You can only call this endpoint with POST, when you pass the arguments in the body. Once you want to make a GET request, you need to pass arguments in the query string - thereby changing the URI. You could request GET /rpc/options_test_overloaded_at_least_one_non_volatile?num=1 - but that's something that OPTIONS /rpc/options_test_overloaded_at_least_one_non_volatile?num=1 needs to return. And this options request should then return OPTIONS,GET,HEAD only - no POST, because with POST you can't pass arguments via query string.

This makes the whole thing a lot more complicated, because you need to take the actual arguments passed via query string into account. The routing-refactor will make this a lot easier, so I suggest we wait for that.

@steve-chavez
Copy link
Member

So the assumption of the PR is that OPTIONS only identifies the URL-path(/path) capabilities. But OPTIONS actually identifies the resource capabilities(the whole URL with query parameters). The RFC does mention this.

I guess the assumption comes from seeing examples in the wild, like this one from Spring REST:

OPTIONS /SpringRestExample/api/rest/employee-management/employees/1
 
Allow:           GET,DELETE,PUT,OPTIONS

You could request GET /rpc/options_test_overloaded_at_least_one_non_volatile?num=1 - but that's something that OPTIONS /rpc/options_test_overloaded_at_least_one_non_volatile?num=1 needs to return. And this options request should then return OPTIONS,GET,HEAD only - no POST, because with POST you can't pass arguments via query string.

This makes me realize that OPTIONS for our table/view paths are also incomplete, because currently:

OPTIONS /projects?id=eq.1

Allow: OPTIONS,GET,HEAD,POST,PUT,PATCH,DELETE

And the Allow should omit the POST there.

@wolfgangwalther
Copy link
Member

So the assumption of the PR is that OPTIONS only identifies the URL-path(/path) capabilities. But OPTIONS actually identifies the resource capabilities(the whole URL with query parameters). The RFC does mention this.

Thanks for putting it into much better words than I did :).

This makes me realize that OPTIONS for our table/view paths are also incomplete, because currently:

OPTIONS /projects?id=eq.1

Allow: OPTIONS,GET,HEAD,POST,PUT,PATCH,DELETE

And the Allow should omit the POST there.

Uh. Absolutely right. And PUT should only be there, when the PK is given in the query string, too.

@laurenceisla
Copy link
Member Author

According to the discussion above, the routing refactor is needed to fix the issue. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants