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: compare DateTime in newUpIfMissing #991

Merged
merged 1 commit into from
May 2, 2024

Conversation

adamcikado
Copy link
Contributor

@adamcikado adamcikado commented Jan 27, 2024

πŸ”— Linked issue

#1018

❓ Type of change

  • 🐞 Bug fix (a non-breaking change that fixes an issue)

πŸ“š Description

Resolves #1018

Fixes bug in updateOrCreateMany, fetchOrCreateMany and fetchOrNewUpMany (which uses newUpIfMissing) where it does not compare DateTime values correctly and therefore creates duplicate rows.
https://github.com/adonisjs/lucid/blob/develop/src/orm/base_model/index.ts#L212

πŸ“ Checklist

  • I have linked an issue or discussion.

@RomainLanz RomainLanz closed this Mar 26, 2024
@RomainLanz RomainLanz reopened this Mar 26, 2024
@RomainLanz
Copy link
Member

Hey! πŸ‘‹πŸ»

Thanks for your contribution!
It seems that the fail, may you link into it?

@adamcikado
Copy link
Contributor Author

@RomainLanz updated. I had to change transformValue method so drivers get Date object instead in queries.

@thetutlage
Copy link
Member

I see there is a bigger problem here because the newUpIfMissing will fail for any sort of any non-literal values.

Yes, we might be able to fix it for DateTime instances, but the same check will fail if some property holds an array of values or an object.

src/database/query_builder/chainable.ts Outdated Show resolved Hide resolved
src/database/query_builder/insert.ts Outdated Show resolved Hide resolved
src/orm/base_model/index.ts Outdated Show resolved Hide resolved
@adamcikado adamcikado force-pushed the develop branch 3 times, most recently from 0dcbaa9 to 54033e0 Compare March 30, 2024 13:39
@adamcikado
Copy link
Contributor Author

@thetutlage @RomainLanz I managed to get it working for all dialects. The comments are incorporated.

However this is not the cleanest solution when it comes to transforming the DateTime values, it should be ok for now. I think it would be best to transform all values passed to .where using the same prepare logic on column as when inserting the data. I can create a PR for that after this one, if it makes sense to you.

@thetutlage thetutlage closed this May 1, 2024
@thetutlage thetutlage reopened this May 1, 2024
@thetutlage
Copy link
Member

@adamcikado There is a type-checking error in the code. Can you please fix that and then we can release this change.

Sorry it took a while :)

@adamcikado
Copy link
Contributor Author

@thetutlage Thank you and fixed!

Regarding what I mentioned about transforming values passed to .where() in the last comment, what's the best place to discuss it (if it makes sense to you)?

@thetutlage thetutlage merged commit 94b666a into adonisjs:develop May 2, 2024
16 checks passed
@thetutlage
Copy link
Member

Regarding what I mentioned about transforming values passed to .where() in the last comment, what's the best place to discuss it (if it makes sense to you)?

I think the simplest option will be to add whereDate, whereDay, whereTime helpers in the query builder and do not perform automatic transformations.

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.

If value is DateTime in newUpIfMissing, duplicate rows are created even if the value is the same
3 participants