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

Require ids in find methods #807

Merged
merged 2 commits into from
Apr 22, 2024
Merged

Require ids in find methods #807

merged 2 commits into from
Apr 22, 2024

Conversation

paulomarg
Copy link
Contributor

@paulomarg paulomarg commented Apr 19, 2024

WHY are these changes introduced?

Closes #756

When all ids evaluated to falsey values in the REST resources' find() methods, we could end up defaulting to the non-id specific versions of a request - e.g. /products/<id>.json became /products.json because we treated these ids as optional.

However, typically in REST APIs, whenever we are GET-ing a specific item you'll need to supply its id, so it didn't make sense for find() to do an id-less query.

WHAT is this pull request doing?

Making it possible to trigger an error if we're missing an id by passing in a parameter to baseFind, and updating all find() methods to pass that parameter in.

I'm considering this a bug fix because those requests were never supposed to be fired in the first place and would cause unpredictable behaviour.

To review the "manual" changes, use this link.

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)

Checklist

  • I have used yarn changeset to create a draft changelog entry (do NOT update the CHANGELOG.md files manually)
  • I have added/updated tests for this change

@paulomarg paulomarg requested a review from a team as a code owner April 19, 2024 20:46
@paulomarg
Copy link
Contributor Author

Note: I search-replaced the requireIds param in the actual classes, so they should all be the same. Just skimming that code should be enough.

Copy link
Contributor

@sle-c sle-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, confirmed that .all methods have requireIds: false 🙇

@paulomarg paulomarg merged commit 8976240 into main Apr 22, 2024
19 checks passed
@paulomarg paulomarg deleted the require_ids_in_find_methods branch April 22, 2024 12:52
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.

Rest resource .find returns last seen object when passing id = NaN
2 participants