adonisjs / lucid

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

fix: apply relationship constraints when using sub query in whereIn condition #1037

Closed adamcikado closed 3 months ago

adamcikado commented 5 months ago

🔗 Linked issue

1036

❓ Type of change

📚 Description

Resolves #1036

📝 Checklist

adamcikado commented 5 months ago

The pipeline errors do not seem to be related to this PR.

thetutlage commented 5 months ago

I am little confused on how these changes fixes that issue. I can see that you have abstracted this inline logic https://github.com/adonisjs/lucid/pull/1037/files#diff-76f631b7273d470789eccc7dbf79045349516bc0178df8de8dfa4425d2be1dd4R233-R234 to its own function. But how does that fixes the issue.

PS: I have not scanned the entire source code yet, so maybe I am missing something :)

adamcikado commented 5 months ago

@thetutlage The whereIn method uses transformValue that converts Chainable using following logic:

if (value instanceof Chainable) {
      value.applyWhere()
      return value.knexQuery
    }

So there is applyConstraints() missing for relationships. I've added a toKnex method to Chainable which calls applyWhere and returns knexQuery. I override this method in relation query builder (like other methods do), so that toKnex also calls applyConstraints.

thetutlage commented 5 months ago

Okay, let me give it a run locally to ensure nothing else breaks.

adamcikado commented 4 months ago

@thetutlage any luck with testing this?

In my opinion, it's important issue and should be fixed. We lost some production data because of this a while ago and had to restore the table. We had something like this in the codebase:

Table1.query()
.where('table2_id', record.related('table2').query().select('id'))
.delete()

Which deleted all records in the table.