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

Fix BigInt primary key routing #20744

Merged
merged 5 commits into from
Dec 14, 2023
Merged

Conversation

wasimTQ
Copy link
Contributor

@wasimTQ wasimTQ commented Dec 14, 2023

Scope

What's changed:

  • Fixes the routing error when opening a bigint value

Potential Risks / Drawbacks

Review Notes / Questions

  • I'd like to know whether it fixes the error and are there any other aspects get affected by this change

Fixes #20288

Copy link

changeset-bot bot commented Dec 14, 2023

🦋 Changeset detected

Latest commit: f116552

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@directus/api Patch
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@wasimTQ wasimTQ changed the title query bigint field with correct value Fix BigInt primary key routing Dec 14, 2023
@wasimTQ
Copy link
Contributor Author

wasimTQ commented Dec 14, 2023

@br41nslug Can you review it?

Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

Thanks @wasimTQ!

The fix looks sensible 👍 (Although, I'm not able to reproduce the issue #20288 in particular).
We need to add a new test case for this before we can merge.

Copy link
Member

@br41nslug br41nslug left a comment

Choose a reason for hiding this comment

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

Using the native BigInt type solves the issue nicely on my end :D

Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

Can confirm as well 🚀
I'd still like to have this covered by a test.

@wasimTQ
Copy link
Contributor Author

wasimTQ commented Dec 14, 2023

(Although, I'm not able to reproduce the issue #20288 in particular).

@paescuj I think it's because of this issue #20287. I have a working solution of it tho. I'm currently working on that atm.

@wasimTQ
Copy link
Contributor Author

wasimTQ commented Dec 14, 2023

Can confirm as well 🚀 I'd still like to have this covered by a test.

What should the test cover? Just that the BigInt field is returning in the whereClause and the number is not changed?
I'll work on that. I'm still kinda weak in the testing aspect. So I might need some help there.

@br41nslug
Copy link
Member

@paescuj I would love to see tests for this too however there is unfortunately a reason there are no tests for this currently 😬 This specific logic is hidden away in 3 levels deep of nested function which makes testing this separately very difficult if not impossible without refactoring
image

if (['integer', 'float', 'decimal'].includes(type)) {

@paescuj
Copy link
Member

paescuj commented Dec 14, 2023

@paescuj I think it's because of this issue #20287. I have a working solution of it tho. I'm currently working on that atm.

Nope, I've inserted the value directly into the DB, but apparently it needs to be significantly larger than integer max safe.
That being said, I've noticed #20287 too, so I'm glad to hear you're working on that ❤️

@paescuj
Copy link
Member

paescuj commented Dec 14, 2023

@paescuj I would love to see tests for this too however there is unfortunately a reason there are no tests for this currently 😬 This specific logic is hidden away in 3 levels deep of nested function which makes testing this separately very difficult if not impossible without refactoring

Hmm, I would have thought it to be feasible by passing in a corresponding schema

const FAKE_SCHEMA: SchemaOverview = {
and root filter
describe('applyFilter', () => {

@br41nslug
Copy link
Member

In that case I hope im wrong 😄

@wasimTQ
Copy link
Contributor Author

wasimTQ commented Dec 14, 2023

@paescuj Was the test I wrote good?

@paescuj
Copy link
Member

paescuj commented Dec 14, 2023

@paescuj Was the test I wrote good?

Yes, thank you! ❤️

@paescuj paescuj enabled auto-merge (squash) December 14, 2023 17:12
@paescuj paescuj merged commit 1264338 into directus:main Dec 14, 2023
4 checks passed
@github-actions github-actions bot added this to the Next Release milestone Dec 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

BigInt primary key routing in the App
3 participants