search

adonisjs / lucid

AdonisJS SQL ORM. Supports PostgreSQL, MySQL, MSSQL, Redshift, SQLite and many more
https://lucid.adonisjs.com/
MIT License
1.07k stars 191 forks source link

fix: compare DateTime in newUpIfMissing #991

Closed adamcikado closed 4 months ago

adamcikado commented 8 months ago

🔗 Linked issue

1018

❓ Type of change

📚 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

RomainLanz commented 6 months ago

Hey! 👋🏻

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

adamcikado commented 6 months ago

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

thetutlage commented 6 months ago

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.

adamcikado commented 6 months ago

@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 commented 5 months ago

@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 commented 5 months ago

@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 commented 4 months ago

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.