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.

Sign up for GitHub

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

Jump to bottom

AIP-84 Allow only positive limit and offset #44109

Merged
merged 5 commits into from
Nov 18, 2024
Merged

AIP-84 Allow only positive limit and offset #44109

merged 5 commits into from
Nov 18, 2024
+92 −5

Conversation

rawwar
Copy link
Collaborator

@rawwar rawwar commented Nov 17, 2024

As of now, OffsetFilter and LimitFilter allow the setting of negative values. With this PR, negative values will raise a 400 error.

@rawwar rawwar added the AIP-84 Modern Rest API label Nov 17, 2024
pankajastro
pankajastro reviewed Nov 17, 2024
View reviewed changes
Copy link
Member

@pankajastro pankajastro left a comment

Choose a reason for hiding this comment

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

Could you please add a test for this?

@rawwar
Copy link
Collaborator Author

rawwar commented Nov 17, 2024 •
edited
Loading

Could you please add a test for this?

We aren't adding tests specifically for filters. But, I'm adding endpoints which will utilize this and have tests. - #43506

I'll add a few to existing endpoints to verify the tests. Thanks

pierrejeambrun
pierrejeambrun reviewed Nov 18, 2024
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we don't have to do that ourselves, pydantic has a NonNegativeInt which is equivalent to >= 0. Since FastAPI has a great integration with FastAPI, maybe just changing the type to NonNegativeInt can make the trick, preventing us from adding custom code.

Also most likely, type error / validation error are by default 422 Validation Error and not 400. Just to stay consistant that might be converted to 422. (But if using native pydantic type, it will be automatically raised anyway).

@rawwar rawwar force-pushed the kalyan/AIP-84/limit_and_offset_filter branch from f0ea194 to 32df34e Compare November 18, 2024 14:05
pierrejeambrun
pierrejeambrun approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Nice

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Nov 18, 2024 •
edited
Loading

Wating for #44138 before merging. (This CI is shorter to run, and PR is easier to update if there are merge conflicts)

@pierrejeambrun pierrejeambrun merged commit 49daa6c into apache:main Nov 18, 2024
45 checks passed
kandharvishnu pushed a commit to kandharvishnu/airflow that referenced this pull request Nov 19, 2024
@rawwar @kandharvishnuu
AIP-84 Allow only positive limit and offset (apache#44109)
eb9323f 
* limit and offset can't be negative

* add tests

* refactor

* use NonNegativeInt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pankajastro pankajastro pankajastro approved these changes

@pierrejeambrun pierrejeambrun pierrejeambrun approved these changes

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy ephraimbuddy is a code owner

Assignees
No one assigned
Labels
AIP-84 Modern Rest API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rawwar @pierrejeambrun @pankajastro