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

filterMany has no effect on OneToOne #3455

Closed
artemik opened this issue Aug 11, 2024 · 3 comments · Fixed by #3473
Closed

filterMany has no effect on OneToOne #3455

artemik opened this issue Aug 11, 2024 · 3 comments · Fixed by #3473
Assignees
Milestone

Comments

@artemik
Copy link

artemik commented Aug 11, 2024

Expected behavior

For OneToOne relationships, I expect either of these:

  • .filterMany(...) call has effect, i.e. applies corresponding filtering clauses in the resulting SQL query;
  • or .filterMany(...) function isn't available at all in the generated query bean on the field corresponding to OneToOne

Actual behavior

The .filterMany(...) function is available on a OneToOne query bean field, but has no effect, i.e. corresponding filtering (WHERE) clauses aren't present in the resulting SQL query. As if filterMany isn't there.

Ebean 15.5.0

@rbygrave
Copy link
Member

rbygrave commented Sep 2, 2024

filterMany has no effect on OneToOne

This is correct and also the expected behaviour.

That is, it is not clear that the "Fix" is worth the extra complexity "cost".

Query beans are an interesting design problem, I think we have done a good job. There are 2 types being "Root query beans" [extends QueryBean] and "Associated beans" [extend TQAssocBean]. These "Associated beans" are used to support both *ToOne and *ToMany associations/relationships. This simplification means that we have filterMany on ToOne relationships which isn't ideal but does keep things simpler.

A "Fix" for this would require 2 types of "Associated Beans" and then for each entity type we would have the 3 query beans types - the QueryBean (root level one), a "Assoc ToMany" and a "Assoc ToOne" one ... and filterMany() would only be on the "Assoc ToMany" query bean.

This might be a good idea and something to try but to date I haven't felt that it was worth pursuing or something that was a high priority.

@artemik
Copy link
Author

artemik commented Sep 2, 2024

I see the reasonings. You might be right, but I can't really comment on it since I'm not an expert on the library internals.

And I agree, for OneToOne relationships, "no effect" is probably more logical behavior for filterMany than something else.

But at the same time, this method exists and confuses users. Especially, "Relationship Pair" docs mention that OneToOne and OneToMany are treated kind of the same in Ebean internals (or at least it sounds like that to me), so somebody might expect that filterMany will actually work. When instead you actually see filterMany silently fail, you start thinking what else might this library allow me to do what I actually shouldn't do (shooting myself in the foot)? Avoiding these things would allow more trust in the library as a whole. Maybe this filterMany is currently just the only thing like that, but still.

For example, it caught me when I refactored some OneToMany relationship to OneToOne (don't ask why). Because there was no compilation error, I realized only at runtime (after a bit of puzzle, becase there were no errors) that I still have filterMany that isn't supposed to be there anymore.

As a mitigation, is it possible to throw a runtime exception somehow when filterMany is used on OneToOne? It seems it should be possible, because similarly somehow Ebean currently detects that filterMany shouldn't do anything because it's being called on OneToOne, so could additionally throw an exception there?

Or at least could you add a warning in javadoc?

@rbygrave
Copy link
Member

rbygrave commented Sep 2, 2024

Great points, thanks.

For me, I am aware that I don't see issues like this that hit normal users of ebean [because I know the internals and the background design decisions etc].

I'll have another look to see if we can get this without resulting in too much complexity.

rbygrave added a commit that referenced this issue Sep 9, 2024
…ly on ToMany relationships

Changes the query bean code generation for "Associated beans" to have separate AssocOne and AssocMany for *ToOne and *ToMany relationships.

Moves the filterMany() expressions such that they are only available on the *ToMany relationships.
rbygrave added a commit that referenced this issue Sep 9, 2024
…ly on ToMany relationships

Use protected helper methods on TQAssocBean for the filterMany() methods generated onto AssocMany beans.
@rbygrave rbygrave self-assigned this Sep 9, 2024
rbygrave added a commit that referenced this issue Sep 17, 2024
…ovement

#3455 - Improve query beans such that filterMany() expressions are only on ToMany relationships
@rbygrave rbygrave added this to the 15.6.0 milestone Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants