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

Panache EntityBase should flush on the right entity manager #24470

Merged
merged 1 commit into from
Mar 26, 2022

Conversation

loicmathieu
Copy link
Contributor

Fixes #24394

@gsmet
Copy link
Member

gsmet commented Mar 22, 2022

You saw me coming but no tests? We should at least have a test that calls the method to make sure it doesn't fail altogether.

@gsmet gsmet changed the title Panache EntityBase should flush on the right entity managerf Panache EntityBase should flush on the right entity manager Mar 22, 2022
@loicmathieu
Copy link
Contributor Author

@gsmet there is already test on this method, I'm not sure we can easily check that we flush the right entity manager. The current test call the method but made no assertion.

This is also a breaking change but I don't think there is a hight risk that someone rely on the wrong behaviour of the current situation.

@gsmet
Copy link
Member

gsmet commented Mar 22, 2022

there is already test on this method

OK, good, then.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 22, 2022
@loicmathieu
Copy link
Contributor Author

Well, hum, I'm a little ashame as tests didn't pass and I didn't notice it because I use mvn clean install instead of mvn clean test ont the tests :(.
I prepare a fix, sorry for that.

@quarkus-bot

This comment has been minimized.

@loicmathieu
Copy link
Contributor Author

@gsmet it can work with this hacky code (I need to add safeguard to it to cover possible corner cases):

     */
    default void flush() {
        Class<?> entityClass = findEntityClass(this.getClass());
        INSTANCE.getEntityManager(entityClass).flush();
    }

    default Class<?> findEntityClass(Class<?> theClass) {
        if (theClass.getGenericInterfaces().length > 0) {
            return ((Class<?>) ((ParameterizedType) theClass.getGenericInterfaces()[0]).getActualTypeArguments()[0]);
        }

        // if the type is not generic we climb up the hierarchy of classes
        return findEntityClass(theClass.getSuperclass());
    }

This issue is that I need to find the entity class wich is the parameterized type of the PanacheRepository/PanacheRepositoryBase interface or any custom interface or abstract type that a user may define on top of them ...

Even if it's less practicle to use, I wonder if we should deprecate flush() and introduce flush(Class<? extends PanacheEntityBase>)` instead as finding the right entity class may be tricky ?

@gsmet
Copy link
Member

gsmet commented Mar 22, 2022

Don't we already have code that could take care of that when generating the classes? I mean it's not a good idea to resolve that at execution time given we know what it will be at build time.
How do we do it for the other methods?

And no, we don't want a flush(Class), especially in Panache when things should be simple.

@loicmathieu
Copy link
Contributor Author

@gsmet yes but it didn't work for flush, I'll dig a little more, I don't want a flush(class) neither.

@loicmathieu loicmathieu marked this pull request as draft March 22, 2022 14:12
@loicmathieu loicmathieu marked this pull request as ready for review March 25, 2022 10:25
@loicmathieu
Copy link
Contributor Author

@gsmet the current PR should fix the issue, the flush method on the Java Panache repository was not correct (the Kotlin one was) as it didn't use the bytecode enhancement getEntityManager method.

@gastaldi gastaldi merged commit e894802 into quarkusio:main Mar 26, 2022
@quarkus-bot quarkus-bot bot added this to the 2.9 - main milestone Mar 26, 2022
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Mar 26, 2022
@gsmet gsmet modified the milestones: 2.9 - main, 2.8.0.Final Mar 28, 2022
@gsmet gsmet removed this from the 2.8.0.Final milestone May 5, 2022
@gsmet gsmet added this to the 2.7.6.Final milestone May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panache: give a flush() method that pinpoints the right PU
3 participants