Change 205861 had a related patch set uploaded (by Mobrovac):
Change route to /png instead of .png

https://gerrit.wikimedia.org/r/205861

I feel that we should keep .png because

• the difference is purely philosophical, and does not affect actual

functioning of the service

• this is not a blocking issue, and can be changed later
• the /a/b/c/file.png is by far more common pattern used in live services
• other node code, like the mapbox studio, uses express npm, and parses

path just fine with /.../{name}.{format} patern

• saving file does not require any headers - user may view just the image

in browser with url (header makes the browser download file), and user may
save it by right clicking it and won't need to be renamed

Thus, I don't see this as a block, nor needed in general.

Plus, when discussed with @GWicke, we decided to use rest proxy mostly
because it was not imposing new rules. Please keep it flexible, and not
order dependent. If you must, we can add additional /p/, but keep .png

In T97123#1233551, @Yurik wrote:

I feel that we should keep .png because

• the difference is purely philosophical, and does not affect actual

functioning of the service

It's about a standard, not its implementation.

• this is not a blocking issue, and can be changed later

Not really. Once you publish an API, you have to change the version if you want to change it. Really, resolving such issues before putting it into production is rather important for the overall consistency of our API, even though it may seem as a minor and non-functional one to you.

• the /a/b/c/file.png is by far more common pattern used in live services

They do not need to be consistent with WM's API, now, do they?

• other node code, like the mapbox studio, uses express npm, and parses

path just fine with /.../{name}.{format} patern

Again, it's about standard and consistency, not about implementation

• saving file does not require any headers - user may view just the image

in browser with url (header makes the browser download file), and user may
save it by right clicking it and won't need to be renamed

This is a missing feature in the service. It should advertise the content type it sends back to the client. Relying on the fact that the browser will render it correctly is wrong.

Thus, I don't see this as a block, nor needed in general.

Plus, when discussed with @GWicke, we decided to use rest proxy mostly
because it was not imposing new rules. Please keep it flexible, and not
order dependent. If you must, we can add additional /p/, but keep .png

Setting up a proxy route in RB is completely another discussion, but, FTR, the same discussion/concern applies there. It is even more important to have a consistent API in RB.

Boy how i love bike shading :)

In T97123#1233567, @mobrovac wrote:

In T97123#1233551, @Yurik wrote:

I feel that we should keep .png because

• the difference is purely philosophical, and does not affect actual

functioning of the service

It's about a standard, not its implementation.

The common Internet practice is to serve png images with .png URLs.

• this is not a blocking issue, and can be changed later

Not really. Once you publish an API, you have to change the version if you want to change it. Really, resolving such issues before putting it into production is rather important for the overall consistency of our API, even though it may seem as a minor and non-functional one to you.

I agree that changing is always a pain. But we are talking about an extra Express routing rule, both patterns could be handled if needed.

• the /a/b/c/file.png is by far more common pattern used in live services

They do not need to be consistent with WM's API, now, do they?

We should be consistent with the world, not the other way around :)

• other node code, like the mapbox studio, uses express npm, and parses

path just fine with /.../{name}.{format} patern

Again, it's about standard and consistency, not about implementation

Again, agree, but the world has it more frequently :)

• saving file does not require any headers - user may view just the image

in browser with url (header makes the browser download file), and user may
save it by right clicking it and won't need to be renamed

This is a missing feature in the service. It should advertise the content type it sends back to the client. Relying on the fact that the browser will render it correctly is wrong.

Of course the content-type:image/png is being sent. I mean the other headers you mentioned in the email - we tried it with @csteipp make it for very bad system - if you copy/paste the actual link to the image, browser automatically starts downloading it. See headers returned by google maps tiles.

Thus, I don't see this as a block, nor needed in general.

Plus, when discussed with @GWicke, we decided to use rest proxy mostly
because it was not imposing new rules. Please keep it flexible, and not
order dependent. If you must, we can add additional /p/, but keep .png

Setting up a proxy route in RB is completely another discussion, but, FTR, the same discussion/concern applies there. It is even more important to have a consistent API in RB.

As I said before - if you MUST have an extra param between the slashes, add a /p/ to the path (e.g. /{domain}/v1/p/{title}/{revid}/{id}.png or /{domain}/v1/{title}/{revid}/p/{id}.png), but keep the .png. I have seen internet using http://example.com/a?p=1&q=2&r=3 format, and i have seen the /a/b/c/d.png, but i have not seen /a/b/c without params serving image (i'm sure they exist, just not very common)

In T97123#1233672, @Yurik wrote:

Boy how i love bike shading :)

Everybody does, right? :) I know it feels like bike-shedding to you, but it's important we get right ;)

The common Internet practice is to serve png images with .png URLs.

I think we are talking about two different standards here. I am concerned with the API path layout, for which we have chosen to go with REST.

I agree that changing is always a pain. But we are talking about an extra Express routing rule, both patterns could be handled if needed.

At least we agree on something :) Yes, we could add an extra route, but why add boiler-plate code at all since we are discussing about the issue here and now?

They do not need to be consistent with WM's API, now, do they?

We should be consistent with the world, not the other way around :)

Uf, my comment came across incredibly wrong, apparently. What I really meant was what I mention above, about talking about different standards.

Of course the content-type:image/png is being sent. I mean the other headers you mentioned in the email - we tried it with @csteipp make it for very bad system - if you copy/paste the actual link to the image, browser automatically starts downloading it. See headers returned by google maps tiles.

You are right, the service does send that header, my bad. I am not sure I understand your point about downloads actually. Are you saying that with /png browsers would not display the image?

As I said before - if you MUST have an extra param between the slashes, add a /p/ to the path (e.g. /{domain}/v1/p/{title}/{revid}/{id}.png or /{domain}/v1/{title}/{revid}/p/{id}.png), but keep the .png. I have seen internet using http://example.com/a?p=1&q=2&r=3 format, and i have seen the /a/b/c/d.png, but i have not seen /a/b/c without params serving image (i'm sure they exist, just not very common)

Hehe, but adding /p/ misses the point of this ticket entirely. The problem here is that mixing a variable and a static part in the same URL path component is a no-go according to REST. Or mixing two or more variables, for that matter.

I would definitely like to keep the .png. For better or worse, all browsers still do some content sniffing, and the file extension is one more way we can encourage the browser to get the right interpretation.

@mobrovac, let's treat the dot as a path separator, in which case it fully
falls into resty paradigm.

I agree with @csteipp re sniffing. Also, when file is given as a pepper
name.ext, if user right clicks on the image and clicks save, the file will
be saved with extension. Without it, usability is much worse - user will
always have to add extension manually. Thus, please make tech work for ppl
))
mobrovac added a comment.

In https://phabricator.wikimedia.org/T97123#1233672, @Yurik wrote:

Boy how i love bike shading :)

Everybody does, right? :) I know it feels like bike-shedding to you, but
it's important we get right ;)

The common Internet practice is to serve png images with .png URLs.

I think we are talking about two different standards here. I am concerned
with the API path layout, for which we have chosen to go with REST.

I agree that changing is always a pain. But we are talking about an extra

Express routing rule, both patterns could be handled if needed.

At least we agree on something :) Yes, we could add an extra route, but why
add boiler-plate code at all since we are discussing about the issue here
and now?

They do not need to be consistent with WM's API, now, do they?

We should be consistent with the world, not the other way around :)

Uf, my comment came across incredibly wrong, apparently. What I really
meant was what I mention above, about talking about different standards.

Of course the content-type:image/png is being sent. I mean the other

headers you mentioned in the email - we tried it with @csteipp make it for
very bad system - if you copy/paste the actual link to the image, browser
automatically starts downloading it. See headers returned by google maps
tiles.

You are right, the service does send that header, my bad. I am not sure I
understand your point about downloads actually. Are you saying that with
/png browsers would not display the image?

As I said before - if you MUST have an extra param between the slashes,

add a /p/ to the path (e.g. /{domain}/v1/p/{title}/{revid}/{id}.png or
/{domain}/v1/{title}/{revid}/p/{id}.png), but keep the .png. I have seen
internet using http://example.com/a?p=1&q=2&r=3 format, and i have seen
the /a/b/c/d.png, but i have not seen /a/b/c without params serving image
(i'm sure they exist, just not very common)

Hehe, but adding /p/ misses the point of this ticket entirely. The
problem here is that mixing a variable and a static part in the same URL
path component is a no-go according to REST. Or mixing two or more
variables, for that matter.

TASK DETAIL

https://phabricator.wikimedia.org/T97123

REPLY HANDLER ACTIONS

Reply to comment or attach files, or !close, !claim, !unsubscribe or

!assign <username>.

EMAIL PREFERENCES

https://phabricator.wikimedia.org/settings/panel/emailpreferences/

To: mobrovac
Cc: csteipp, gerritbot, GWicke, Yurik, mobrovac, Aklapper, Eevans, Hardikj,
marcoil, Ricordisamoa, RobLa-WMF, bd808

At a technical level we can handle file extensions as part of the id without issues. I think the only issue is about not using the file extension to distinguish entry points.

With something like /path/to/{format}/{id} (ex: /path/to/png/deadbeef.png), it is possible to list the available formats at /path/to/. We can also clearly document the expected content types for each route, and later enforce sanitization for SVGs.

In T97123#1234031, @GWicke wrote:

With something like /path/to/{format}/{id} (ex: /path/to/png/deadbeef.png), it is possible to list the available formats at /path/to/. We can also clearly document the expected content types for each route, and later enforce sanitization for SVGs.

Agreed, this seems like a good compromise to me. I would add here that using such a path outline would allow the service to accept IDs both with and without the suffixed extension.

@Yurik , @csteipp ?

In T97123#1238103, @mobrovac wrote:

In T97123#1234031, @GWicke wrote:

With something like /path/to/{format}/{id} (ex: /path/to/png/deadbeef.png), it is possible to list the available formats at /path/to/. We can also clearly document the expected content types for each route, and later enforce sanitization for SVGs.

Agreed, this seems like a good compromise to me. I would add here that using such a path outline would allow the service to accept IDs both with and without the suffixed extension.

@Yurik , @csteipp ?

If you're saying that the type is both in the path and in the extension, then that's fine for security.

Change 205861 merged by Mobrovac:
Change route to include the format as well

https://gerrit.wikimedia.org/r/205861