Skip to content

Conversation

@gsmet
Copy link
Member

@gsmet gsmet commented Jan 22, 2019

@gbadner
Copy link
Contributor

gbadner commented Jan 23, 2019

This looks fine to me. It would be good to have a test of something that was getting enhanced, when it shouldn't be.

From the topical guide, enabling field access as in:

<enableExtendedEnhancement>false</enableExtendedEnhancement>

"Field access is not enhanced by default, because it can potentially trigger enhancement of code outside the entities. Other capabilities are enabled by default."

I'm not sure if field access is tested though.

@gsmet
Copy link
Member Author

gsmet commented Jan 23, 2019

So the thing is that this change doesn't modify the previous behavior: if your entity was not enhanced, it's still not enhanced, it's just that you're not doing useless work.

The initial issue was in 2 parts:

  • in 5.3, we wrongly return the bytes even if we don't really enhance the entity: that's why the class is rewritten and logged, even if we don't enhance it. That was fixed during the 5.4 cycle and it needs to be backported. I'm working on a backport of all the ByteBuddy related fixes (we have this issue and a couple of others leading to performance issues that I consider being a bug - note that this includes an upgrade of ByteBuddy as there was an issue in BB itself leading to performance problems with entities containing quite a lot of properties);
  • the second issue is that we created this transformer too early. It doesn't enhance anything, we are just doing work we shouldn't do (and it was causing an enhancement issue on a class, I suspect a BB bug specific to this particular class but I will have to confirm it once I get the class). With this PR, we don't manipulate the class early so we should avoid this issue.

I still think we should wait for a test case before merging this. But if we don't have one, still better to merge it and backport it than not.

@gsmet
Copy link
Member Author

gsmet commented Jan 28, 2019

Merged!

@gsmet gsmet closed this Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants