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

Fail loudly on invalid query params #4274

Open
paulmelnikow opened this issue Oct 31, 2019 · 2 comments
Open

Fail loudly on invalid query params #4274

paulmelnikow opened this issue Oct 31, 2019 · 2 comments
Labels
core Server, BaseService, GitHub auth, Shared helpers

Comments

@paulmelnikow
Copy link
Member

📋 Description

In #4273 I argued that the Shields way is to fail loudly when parameters are incorrect, rather than silently ignoring them. Descriptive error messages point the user as directly as possible to the source of the problem rather than leaving them to wonder.

In particular, it seems like a good idea to return descriptive errors for:

  1. Invalid logos
  2. Invalid color overrides
  3. Styles we have never supported
  4. Other query parameters which are of the incorrect type
    • Though perhaps not, e.g. cacheSeconds which are too low
@paulmelnikow paulmelnikow added the core Server, BaseService, GitHub auth, Shared helpers label Oct 31, 2019
@chris48s
Copy link
Member

chris48s commented Nov 4, 2019

At an abstract level, I do agree with this. However, there are a few practical things to think about:

  1. We'd need to be careful about renaming/removing params. I wouldn't want a change like Rename maxAge to cacheSeconds #3090 (done) or remove the cacheSeconds/maxAge param #3596 (proposed) to be breaking. We can mitigate this ourselves as long as we think about it.
  2. Simple-icons occasionally remove an icon. Usually this is due to the service going away. For example, the google+ icon was removed when google+ was shut down, but it could also happen for example because a company has complained that a logo has violated their terms of use or something. In the case where the service has shut down, a related badge probably also stopped working by the time they removed the icon, so I wouldn't be too concerned by that case. In the other case, lets say (just for the lolz) GitHub complain that simple-icons are violating their logo policy and they remove the icon, I reckon it would be preferable that becomes as opposed to . I'd expect this to be a very uncommon case, but it is worth considering.
  3. I wonder how many badges there are out there in READMEs doing something like https://img.shields.io/github/v/tag/badges/shields?color=cheese which are currently working but would break if we introduced this change. I'd imagine that is the largest category of problem here, but I don't really know. Its also difficult for us to understand the scope of or do much about from our end.

@paulmelnikow
Copy link
Member Author

We'd need to be careful about renaming/removing params. I wouldn't want a change like #3090 (done) or #3596 (proposed) to be breaking. We can mitigate this ourselves as long as we think about it.

Agreed 👍

Simple-icons occasionally remove an icon. Usually this is due to the service going away. For example, the google+ icon was removed when google+ was shut down, but it could also happen for example because a company has complained that a logo has violated their terms of use or something. In the case where the service has shut down, a related badge probably also stopped working by the time they removed the icon, so I wouldn't be too concerned by that case. In the other case, lets say (just for the lolz) GitHub complain that simple-icons are violating their logo policy and they remove the icon, I reckon it would be preferable that becomes as opposed to . I'd expect this to be a very uncommon case, but it is worth considering.

For a bad named logo, one option would be to display something in place of the logo. Not sure any of these are exactly the right thing, but it gives an idea: ⬜⛔💔😶🙈🕳️

Alternatively we could check in a list of formerly used icons. Going forward this could be automatically verified with a unit test. Then we could display something like 💨for deprecated icons and show an error message for completely made-up names. It's too bad we have named and svg logos in the same parameter; it makes it a little harder to know what the intention was when the parameter is neither. Though I suppose any encoded svg logo is going to be longer than a named logo.

I wonder how many badges there are out there in READMEs doing something like https://img.shields.io/github/v/tag/badges/shields?color=cheese which are currently working but would break if we introduced this change. I'd imagine that is the largest category of problem here, but I don't really know. Its also difficult for us to understand the scope of or do much about from our end.

Heh, yea, I can imagine that being a little bit annoying. Though at least there is some upside; when the error badge shows they'll know there is a problem and can fix their URL. I'm sure some people would prefer to just get the default color, though others might prefer to know they have a problem.

We could help this a little by having a transition period when the badge shows with the default color with a little warning on it: ⚠️

And then on the website we could display a prominent message about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth, Shared helpers
Projects
None yet
Development

No branches or pull requests

2 participants