lucid icon indicating copy to clipboard operation
lucid copied to clipboard

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

Open aadamcik opened this issue 1 year ago • 4 comments

🔗 Linked issue

#1036

❓ Type of change

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

📚 Description

Resolves #1036

📝 Checklist

  • [x] I have linked an issue or discussion.

aadamcik avatar Jun 13 '24 16:06 aadamcik

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

aadamcik avatar Jun 13 '24 16:06 aadamcik

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 :)

thetutlage avatar Jun 20 '24 05:06 thetutlage

@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.

aadamcik avatar Jun 20 '24 06:06 aadamcik

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

thetutlage avatar Jun 20 '24 06:06 thetutlage

@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.

aadamcik avatar Jul 08 '24 13:07 aadamcik