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

API V2 Design Review #120

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ImGajeed76
Copy link
Collaborator

API V2 Design Review

This PR outlines the proposed API V2 design. Please review and provide feedback on the changes before we proceed with implementation.

Key feedback areas:

  • Proposed API changes
  • Breaking changes from V1
  • Suggestions for improvement

All feedback will be discussed and incorporated based on team consensus.

Copy link

vercel bot commented Feb 15, 2025

Deployment failed with the following error:

Creating the Deployment Timed Out.

@ImGajeed76
Copy link
Collaborator Author

If you know you want to change something but didn't have time yet, please write a quick comment. If you think its good like that leave a thumbs up.

@solonovamax
Copy link

  • for the bearer token auth type, it isn't fully clear that it can optionally be a JWT token
    smth like

This can be a JWT, or any other kind of token. The type of token used is left up to the implementation.
might be better

  • I don't think that a "default" server api url should be listed in the openapi spec

  • I think the following http codes should all be suported:

    • 400 (Bad Request)
    • 401 (Unauthorized)
    • 403 (Forbidden)
    • 406 (Not Acceptable)
    • 429 (Too Many Requests)
    • 500 (Internal Server Error)
    • 501 (Not Implemented)
  • why was basic authentication removed?

  • somewhere in the auth section for the server information, there should be an optional field that provides a link to a signup page

  • the server information authorization section should list which routes require authorization

  • imo the features section of the server information should be an object rather than an array

  • the coupon query api is still incredibly unclear. it uses a ?q= parameter, and nowhere does it specify where the domain should go.
    it should really just be something like /coupons/site/{domain}/list

  • what happened to the site info route I had?

  • I feel the pagination opject should be shared, and you should use allOf for the schema
    eg. image

  • same applies to the merchant search

  • the PATCH requests are used improperly. according to the HTTP spec, PATCH requests should be sending partial objects, with only the information that is to be updated.
    eg. if the object stored in the database looked something like

    {
        "name": "foo",
        "description": "bar"
    }

    and you were to send a PATCH with

    {
        "name": "foo2",
        "baz": "qux"
    }

    then the afterwards, the object in the database should be

    {
        "name": "foo2",
        "description": "bar",
        "baz": "qux"
    }
  • there is no route for submitting a rating for a coupon

  • a PUT request should really be used to submit things tbh

  • in the filterBy query, something like ?filterBy[discount_value]=, is acceptable, according to the openapi spec

  • what's the difference between a category and a tag for a coupon? why do both exist?

  • BOGO -> BUY_ONE_GET_ONE_FREE

  • an additional OTHER type should be added for the discount type

  • inconsistent use of camel case and snake case. everything should be snake case.

  • honestly, sortBy should be split into sort and sort_order or smth, because all enums have both asc and desc variants.

  • will searchIn ever be actually used?

  • Coupon objects don't have a site field

  • description should be optional for a Coupon

  • there should be a rating field, in addition to the score field
    image

  • imo up_votes and down_votes should be renamed to positive_votes and negative_votes, respectively. but that's just personal preference.

  • minimums and maximums should be set on all relevant fields in the spec

  • imo instead of having
    coupon -(many-to-one)-> merchant
    it should be
    coupon -(many-to-one)-> site -(many-to-one)-> merchant

    where, the site has things like a name, a description, and a domain

  • in many places, there are references to the id of things. however, most times you just need the name of smth. so to avoid additional requests, you should introduce *Reference objects, for example:
    image
    image

  • in the spec, the X-RateLimit-Limit, X-RateLimit-Remaining, and X-RateLimit-Reset headers are only returned for a 200 response. they should always be returned.

  • also, use the responses component list (as well as requestBodies, headers, and parameters):
    image
    image

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

Successfully merging this pull request may close these issues.

2 participants