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

Add not found schema to base services #2632

Open
paulmelnikow opened this issue Jan 4, 2019 · 5 comments
Open

Add not found schema to base services #2632

paulmelnikow opened this issue Jan 4, 2019 · 5 comments
Labels
core Server, BaseService, GitHub auth

Comments

@paulmelnikow
Copy link
Member

This code is from the clojar badge:

    if (Object.keys(json).length === 0) {
      /* Note the 'not found' response from clojars is:
          status code = 200, body = {} */
      throw new NotFound()
    }

We have a bunch of other similar examples, where the schema is written to accommodate the not found case, and then there's code written to detect the alternate case and throw an error.

This responsibility is a kind of schema validation. We'd have clearer guarantees, particularly in the "found" case, if this responsiblity were delegated to Joi.

A really clean way to express this would be to optionally provide two schemas to _requestJson and friends. One would be the schema for the success case (like what we're passing now) and the other a notFoundSchema. If the notFoundSchema matches, we automatically throw NotFound, using the 404 error message.

@paulmelnikow paulmelnikow added the core Server, BaseService, GitHub auth label Jan 4, 2019
@calebcartwright
Copy link
Member

calebcartwright commented Jan 4, 2019

Something I've seen (which I think is along the very same lines), is that for not found scenarios some services will return an HTTP 200 with an html page (that contains the text '404 Not Found'). Would it be feasible to have a notFoundSchema that can handle those html scenarios too?

@paulmelnikow
Copy link
Member Author

some services will return an HTTP 200 with an html page (that contains the text '404 Not Found')

😝

Are these JSON services? notFoundSchema would require valid JSON, though we could have a notFoundRegex which we test if the JSON won't parse, before returning the typical "unparseable json".

@calebcartwright
Copy link
Member

Two places where I've seen this recently are:

Are these JSON services?

😆 They'll return JSON if the query target exists, but doing that HTML nonsense otherwise. I believe the current behavior with those would be invalid response for the badge message when the projects are not found since that HTML isn't valid JSON

@paulmelnikow
Copy link
Member Author

I believe the current behavior with those would be invalid response for the badge message when the projects are not found since that HTML isn't valid JSON

It'd be nice to improve on that.

@calebcartwright
Copy link
Member

calebcartwright commented Jan 4, 2019

Actually I'm wrong 🤦‍♂️ Forgot I added this to return a more meaningful message.

Still would be great if those services would actually respond with a 404 status code or always return JSON so we could use the notFoundSchema approach

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

No branches or pull requests

2 participants