chore: Register JpaFilterConverter in a way that can be overridden #20745
Conversation
| @@ -142,4 +144,10 @@ public ServerEndpointExporter websocketEndpointDeployer() { | |||
| return new VaadinWebsocketEndpointExporter(); | |||
| } | |||
|
|
|||
| @Bean | |||
| @ConditionalOnMissingBean | |||
| public JpaFilterConverter jpaFilterConverter(EntityManager entityManager) { | |||
There was a problem hiding this comment.
I would not want this Bean as flow user. Additionally it might be problematic for projects without JPA because of the EntityManager (so conditional on missing bean is not enough)
There was a problem hiding this comment.
And wait.. is this the code that made "Hilla utterly broken with multiple Data sources"? I'm strict against this in flow!
There was a problem hiding this comment.
Sounds like this is something personal. Can we discuss the merits of the code instead and what it is useful for
There was a problem hiding this comment.
As you can see from the description, it is supposed to fix vaadin/hilla#2569, not introduce it to all Flow users
There was a problem hiding this comment.
It's added to "SpringBootAutoConfiguration" which effects flow.
There was a problem hiding this comment.
Indeed, that is the point of this PR. To add the features to Flow and not only restrict them to Hilla
There was a problem hiding this comment.
Your last two comments are kinda conflicting?
But nevertheless back to code - this JPAFilter probably has to be added in a second AutoConfiguration class to ensure it's only loaded with JPA and Spring Data in use.. or does it have to be a bean at all? It looks just like some helper methods that need access to the EntityManager.. with the "move" to flow and re-creating of all classes.. it's binary incompatible anyway so perfect timing for breaking changes.
Just a quick note on the other code:
- doing lowercasr on a database column by default might be a performance problem with big tables without proper indexes
- the string concatenation for like looks un-sanitized
There was a problem hiding this comment.
Let's see first where #20743 ends up. The original version in Hilla contained quite a bit of magic and this PR now contains way less of it
There was a problem hiding this comment.
Your last two comments are kinda conflicting?
The point is to introduce the features to all Flow users but not to introduce the problem to all Flow users
|
Test Results1 015 files - 144 1 015 suites - 144 1h 23m 2s ⏱️ - 6m 9s For more details on these failures and errors, see this check. Results for commit aa83b9d. ± Comparison against base commit fb02577. This pull request removes 166 tests. |
These were previously in Hilla but are equally useful in Flow applications
Artur-
force-pushed
the
register-jpafilterconverter
branch
from
aa83b9d to
bd81a3b
Compare
|
This should not be needed at all |
Fixes vaadin/hilla#2569