Skip to content

Conversation

@vladmihalcea
Copy link
Contributor

@raphw
Copy link
Member

raphw commented Oct 10, 2018

I would actually also substitute proxyClass.newInstance() by proxyClass.getConstructor().newInstance(). The former method is deprecated as of Java 9 as it does not throw all reflection-related exceptions correctly. Looks good otherwise.

@vladmihalcea
Copy link
Contributor Author

@raphw Good tip. Should we use proxyClass.getConstructor() or proxyClass.getDeclaredConstructor()> Is it that the Proxy class always created a public constructor or does it copy the target class constructor visibility?

@raphw
Copy link
Member

raphw commented Oct 10, 2018

Since we do not attempt accessibility, getConstructor is fine. It requires fewer security manager privileges, too.

I think Byte Buddyis already configured to making the constructor public anyways.

@raphw
Copy link
Member

raphw commented Oct 10, 2018

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vladmihalcea I added some guidance about how to simplify the log thingy.

Let me know if you have any question.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a bit overcomplicated.

I would do that this way:

catch (NoSuchMethodException e) {
    throw LOG.bytecodeEnhancementFailedBecauseOfDefaultConstructor( entityName, t )
}
catch (Throwable t) {
   throw LOG.bytecodeEnhancementFailed( entityName, t );
}

with the log methods being defined by, for instance:

@Message(value = "Bytecode enhancement failed because no public, protected or package-private default constructor was found for entity: %s. Private constructors don't work with runtime proxies!", id = 143)
HibernateException bytecodeEnhancementFailedBecauseOfDefaultConstructor(String entityName, @Cause Exception e);

JBoss-logging is very powerful, let's unleash its power!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsmet I'll try to apply your suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsmet If we skip the log as you suggest, then the log message will look like this:

INFO DefaultLoadEventListener:132 - HHH000327: Error performing load command : org.hibernate.HibernateException: HHH000143: Bytecode enhancement failed because no public, protected or package-private default constructor was found for entity: org.hibernate.jpa.test.callbacks.PrivateConstructorTest$Parent. Private constructors don't work with runtime proxies!

I think it's better to have a separate error log entry as before as it makes it more clear what happened.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

  • as for the first error, I see how the way it is done would lead to this situation: we need to resolve the constructor in postInstantiate(). This way you will have the error once and for all when getting the constructor and you can use the pattern I mentioned.
  • you can then revert the change you made to the error handling in getProxy() and keep it as it was.

As much as possible, we need to avoid the reflection calls at runtime.

Sorry for not seeing it right away.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I'm working on it.

@vladmihalcea vladmihalcea force-pushed the HHH-13020 branch 2 times, most recently from 7572cf8 to 4ad8703 Compare October 10, 2018 13:08
@vladmihalcea
Copy link
Contributor Author

@raphw Thanks, Rafael. I applied your suggestions and will push it.

…tor, the log message is not very clear about the problem
@vladmihalcea vladmihalcea merged commit df3edbd into hibernate:master Oct 10, 2018
@vladmihalcea vladmihalcea deleted the HHH-13020 branch October 10, 2018 14:37
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.

3 participants