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

Javassist Based Oval Builder Validation #18

Merged
merged 3 commits into from
Sep 28, 2016
Merged

Conversation

vjkoskela
Copy link
Member

Javassist plugin integration and class processor for adding OvalBuilder validation. This is a preview only and will not build since the plugin has not been released. Also it is missing implementation of the reflective validation rules. However, I would appreciate any feedback in the meantime as I wrap-up testing of the plugin.

@Override
public void process(final CtClass ctClass) {
try {
final String collectSuperClassViolations;
Copy link
Member

Choose a reason for hiding this comment

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

Some explanation as to why you're collecting (a chain?) of methods to find violations is in order. Also, what happens if the super class wasn't weaved? Note: make sure you have a test for this situation somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

So to be clear it's collection a list of violations not methods to find violations.

If any super class other than the eventual OvalBuilder parent class has not been woven then the validate method defaults to the one in OvalBuilder which uses reflection to analyze the entire class tree.

This may result in some duplicate violations being detected. I will add a test case for this.

Does that address your concern?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that does address my concern. Is there a way that we can prevent that? That'd be really cool ( :

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed issue #20 regarding this.

@vjkoskela vjkoskela changed the title **PREVIEW** Javassist Based Oval Builder Validation Javassist Based Oval Builder Validation Sep 19, 2016
@vjkoskela
Copy link
Member Author

Filed issues #21, #22, #23, #24, #25, #26, #27 and #28 to cover missing validation rules.

@vjkoskela
Copy link
Member Author

** Build broken until plugin version 0.1.0 is in central **

@vjkoskela vjkoskela force-pushed the javassist-plugin branch 5 times, most recently from 227b590 to a4f1924 Compare September 23, 2016 01:52
@vjkoskela
Copy link
Member Author

Fixed javasisst maven plugin and core version references.

@vjkoskela
Copy link
Member Author

So now it looks like it's blocked on the OvalBuilder race condition:

java.lang.NullPointerException
    at net.sf.oval.localization.message.ResourceBundleMessageResolver.getMessage(ResourceBundleMessageResolver.java:95)
    at net.sf.oval.localization.message.ResourceBundleMessageResolver.getMessage(ResourceBundleMessageResolver.java:80)

@vjkoskela
Copy link
Member Author

@BrandonArp Including a local patched version of OVal until the race condition is resolved in the next release of that project. Please take a look at the strategy I selected (e.g. pom.xml).

Copy link
Member

@BrandonArp BrandonArp left a comment

Choose a reason for hiding this comment

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

Wow... that's a messed up way to do things. I don't much like it, to be honest. That said, I do want to get this released. I think you're going to have to shade it as well. Anything that pulls Commons as a dependency will have to resolve the custom snapshot version (they can't) or if they pick up the upstream, it'll be broken with the race condition.

<showDeprecation>true</showDeprecation>
<showWarnings>true</showWarnings>
<compilerArgs combine.self="override">
<!--<arg>-Xlint:all</arg>
Copy link
Member

Choose a reason for hiding this comment

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

What's going on here?

Removing compiler warning override.
@vjkoskela
Copy link
Member Author

@BrandonArp As discussed. This should be good to.

@BrandonArp BrandonArp merged commit 65c60f5 into master Sep 28, 2016
@BrandonArp BrandonArp deleted the javassist-plugin branch September 28, 2016 04:47
@vjkoskela vjkoskela mentioned this pull request Oct 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants