[openstreetmap/openstreetmap-website] Allow `user/preferences/[KEY]` to accept arbitrary keys (PR #3787)

Taylor Smock notifications at github.com
Tue Nov 29 21:24:21 UTC 2022


> But I still don't understand what you are trying to achieve! I thought we'd clarified that we can't use any unescaped . characters from the keys in the urls, since we need . for the format separator.

Sorry, that might be a misunderstanding due to [issuecomment-1318982892](https://github.com/openstreetmap/openstreetmap-website/pull/3787#issuecomment-1318982892). 

In any case, the current code should handle the case where `%2e` gets decoded to `.` prior to the extension being handled.

> If you are trying to support both (optional) format extensions and also dotted-keys in the url, then I believe that's a dead-end. You can't distinguish these two different situations:

I agree. The problem is that it is a ["hard" requirement](https://github.com/openstreetmap/openstreetmap-website/pull/3787#issuecomment-1316963890) to have the ability to support `.fmt` on all endpoints. Which pretty much means I should make certain that `.fmt` works _now_. It is kind of why I [initially](https://github.com/openstreetmap/openstreetmap-website/pull/3787#issuecomment-1310281511) didn't want to support `.fmt` at all.

Using `Accept` request headers is fine. Which leads to the question, why didn't we just use the `Accept` headers in the first place?

> I don't want to see a special-case for the User Preferences API along the lines of "please note that if you have a . in your key and also the characters after the final dot happen to match one of the formats that we support (currently xml and json) then your key cannot be accessed through the direct users preferences API" or something like this. Reading between the lines, I think that's what you might be trying to achieve, but it's not clear to me.

I did not want to do any special casing at all. My original intent was to support arbitrary keys for the endpoint. Unfortunately, if we do support `.fmt`, we will of necessity need to deal with the edge case of adding another format (`.pbf`) or forcing clients to URL encode `.` themselves.

Current options:
* Require that clients URL encode `.` to `%2e` and hope some future update of rails doesn't bork it by url decoding prior to doing the routing
  * I don't like this since it isn't exactly documented behavior. It works right now.
* Don't support `.fmt` on this endpoint and instead require clients use `Accept`
* Don't support `.fmt` on this endpoint and instead allow clients to specify an `Accept` header for error codes
  * Clients can check the return type and go from there (`if [ media_type == "text/plain" ]; do ok_stuff; else handle_error; fi`)
* Special case `.json`/`.xml` so that `.fmt` works and allow `Accept` headers
  * This is where we have to start worrying about `key`, `key.json`, and `key.xml` and _future_ formats. Example: `key.txt`. Unfortunately, we cannot use a regex like `(.+?)(?=(\..*|)$)` This would get _all_ extensions, but would also get stuff like `key.not_an_ext`. We could try limiting the length of the extension `(.+?)(?=(\..{,4}|)$)`, but we still have potential edge cases.

> I don't want you to spend more of your time and effort working on debugging something that turns out to be a dead end or not useful, so I recommend that we clarify what you are trying to do before you jump in with more coding.

Thank you for the consideration. I've been busy the past week or so fixing JOSM tickets prior to (what I had hoped to be) a release week.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/3787#issuecomment-1331330015
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/3787/c1331330015 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20221129/bf989089/attachment.htm>


More information about the rails-dev mailing list