Fix bug with new nested where filters #8005

tjhiggins · 2021-08-02T14:02:49Z

Description of change

Found a bug in the most recent nested where filter feature:
#7805

When combining nested primary keys and non-primary keys the query builder would build an incorrect query.
Included a test case that illustrates the issue.

fixes #8082

Pull-Request Checklist

Code is up-to-date with the master branch
npm run lint passes with this change
npm run test passes with this change
There are new or updated unit tests validating the change
The new commits follow conventions explained in CONTRIBUTING.md

Query builder would generate an incorrect query If you mixed primary and non-primary keys

tjhiggins · 2021-08-02T15:39:50Z

@imnotjames how do I check if an entity is a transformed class vs a plain filter object? struggling with the differences between:

const foundChild = await manager.findOne(Child, { parent: loadedParent });

// Expected: where parent.id = ?

and

.where({
    post: {
        category: {
            id: In([1, 2, 3]),
            name: In(["Foo", "Bar", "Baz"]),
        }
    }
})

// Expected: where category.id in (?, ?, ?) and name in (?, ?, ?)
// Use case: permissions filter on ids, but want to search on name

Edit:
Found what I was looking for:

const isEntityTarget = relation.inverseEntityMetadata.target === entity[key].constructor;

Running tests again now.

tjhiggins · 2021-08-02T16:45:33Z

@imnotjames The node 14 test failed due to a circleci issue - could you please re-run?

imnotjames · 2021-08-02T17:10:56Z

Doesn't the test pass if you write the code as

const isEntityTarget = relation.inverseEntityMetadata.target === entity[key].constructor;

if (isEntityTarget) {
    // .... shortcuts for entity target
}

// If nothing else, just return every property that's being passed to us.
const subPaths = this.createPropertyPath(relation.inverseEntityMetadata, entity[key])
    .map(p => `${path}.${p}`);
paths.push(...subPaths);

tjhiggins · 2021-08-02T17:29:05Z

Doesn't the test pass if you write the code as

const isEntityTarget = relation.inverseEntityMetadata.target === entity[key].constructor;

if (isEntityTarget) {
    // .... shortcuts for entity target
}

// If nothing else, just return every property that's being passed to us.
const subPaths = this.createPropertyPath(relation.inverseEntityMetadata, entity[key])
    .map(p => `${path}.${p}`);
paths.push(...subPaths);

I tried that after I added the isEntityTarget check, but a couple tests failed. Realizing now that those tests might be invalid now and it now generates the correct sql. Let me know if I'm wrong:

6de9327#diff-fc53d8864169b2e2d0b175dc446c164524f59407c457d6b81a0fa1c004d8ca5cR206

Updated with the simple solution and changed those tests.

tjhiggins · 2021-08-02T17:48:18Z

Nvm the simple solution doesn't play nice with updates:

1) embedded > embedded-many-to-one-case1
       owner side
         should insert, load, update and remove entities with embeddeds when embedded entity having ManyToOne relation2:
     EntityColumnNotFound: No entity column "counters.likedUser.id" was found.

Fix bug with yaml mkdir test

tjhiggins · 2021-08-02T19:01:26Z

@imnotjames Ready for another look. Would love your feedback on a better check for hasOnlyPrimaryKeys. Also please note that a couple tests that used to fail now pass - I'm not 100% if they are correct.

imnotjames · 2021-08-09T06:18:16Z

test/functional/connection-options-reader/connection-options-reader.ts

@@ -11,7 +11,7 @@ async function createDotenvFiles() {
 }

 async function createYamlFiles() {
-  await fs.mkdir(path.join(__dirname, "configs/yaml"));
+  await fs.mkdir(path.join(__dirname, "configs/yaml"), { recursive: true });


Why is this change needed?

I wasn't able to run test-fast multiple times in a row. It would complain that the directory already existed.

That's a bigger problem that our tests have than just this. Can you omit this change for now just to simplify this PR & what it touches? If we have to revert or find the cause of something it makes it a whole lot easier if it's one PR for one change.

imnotjames · 2021-08-09T06:21:52Z