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

Relation references within filters #680

Closed
brudil opened this issue Jun 6, 2023 · 5 comments
Closed

Relation references within filters #680

brudil opened this issue Jun 6, 2023 · 5 comments
Labels

Comments

@brudil
Copy link
Collaborator

brudil commented Jun 6, 2023

I'm a bit unsure here if I've hit a runtime error or lack of type-safety here.

Imagine a UserAccount entity with a FK to a Organisation entity.

em.findCount(
  UserAccount,
  {
    // Passing in the reference
    organisation: user.organisation, // ManyToOneReference<UserAccount, Organisation, never>
  }
);

The reference here does not create a TS warning about passing this in to the FilterWithAlias.

When executing this though, we get an error:

fieldName not found on organisation

Digging in to this, it's coming from QueryParser.ts:L158 iterating though the properties of the ManyToOneReference object.

I'm assuming this isn't a runtime bug and that relations should be loaded for use within filters?

@stephenh
Copy link
Collaborator

stephenh commented Jun 6, 2023

Huh, that is weird... that should be:

em.findCount(
  UserAccount, { organisation: user.organization.get } // or `.id`
);

Like the .get is important to turn it from a ManyToOneReference to an Organization (or .id to turn it into a OrganizationId).

I guess technically we could update the runtime to recognize a Relation coming in, and do the .get / .id internally--just really odd that this is not a compile error though.

Not sure whether to double-down on the current/expected API and find/fix the missing compile error, or lean into supporting this syntax as kosher, even if it is unintended.

I think I'll probably start with trying to make sure this is a compile error, because if we're letting this in unintentionally, we're potentially letting other types in unintentionally as well...

Thanks for reporting! I'll dig into this and try to find out what's happening.

@stephenh
Copy link
Collaborator

stephenh commented Jun 9, 2023

Just a small update, I remembered / rediscovered why this happens...

Joist's filter types are what TS calls "weak types" where every field is optional, i.e. firstName?, lastName?, etc.

Because of this, basically any type can technically structurally match an AuthorFilter or BookFilter etc. Even the {} empty type.

I've found a few tickets in the TS repo mentioning that they've tried to recognize these weal types, I believe by heuristics like "there are literally no overlap in keys between these two types".

But I'm not sure why those heuristics aren't working here. I'm still poking around though, and will hopefully find a good approach.

@stephenh
Copy link
Collaborator

stephenh commented Jun 9, 2023

microsoft/TypeScript#16047 is what should have fixed our filter's weak type check, afaiu.

@stephenh
Copy link
Collaborator

stephenh commented Jun 9, 2023

Ah, turns out both AuthorFilter and ManyToOneReference have id fields, so that is just enough for TypeScript's structural typing to consider a book.author-that-is-a-ManyToOneReference to match the AuthorFilter.

If I just manually remove AuthorFilter.id, then I get the expected TS2559: Type 'ManyToOneReference ' has no properties in common with type 'AuthorFilter'. weak type error.

stephenh added a commit that referenced this issue Jun 9, 2023
I think personally I would prefer not allowing relations to be used in
find filters, however b/c ManyToOneReference.id makes it structurally
match the EntityFilter, it seems hard/not worth the complexity to
prevent the ManyToOneReference in the EntityFilter mapped type.

So instead just suppose m2os by calling `.id`.
stephenh pushed a commit that referenced this issue Jun 9, 2023
## [1.88.3](v1.88.2...v1.88.3) (2023-06-09)

### Bug Fixes

* Allow m2os to be used in filters. Fixes [#680](#680). ([#683](#683)) ([4c246fa](4c246fa))
@stephenh
Copy link
Collaborator

stephenh commented Jun 9, 2023

🎉 This issue has been resolved in version 1.88.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants