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

Refactor validation of PluginDescriptors #130

Merged
merged 4 commits into from
Apr 1, 2017
Merged

Conversation

janhoy
Copy link
Member

@janhoy janhoy commented Mar 30, 2017

Was duplicated in each finder, now pluggable.
Did this through an AbstractPluginDescriptorFinder so everyone get the default behavior ootb.
To plug in your own validator you'd need to Override createPluginDescriptorFinder() in your PluginManager.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 15.475% when pulling 014cec2 on cominvent:validator into e9126c4 on decebals:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 15.475% when pulling 014cec2 on cominvent:validator into e9126c4 on decebals:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 15.475% when pulling 014cec2 on cominvent:validator into e9126c4 on decebals:master.

@decebals
Copy link
Member

My idea is to move the validation process from PluginDescriptorFinder to PluginManager, after we found a PluginDescriptor (https://github.com/decebals/pf4j/blob/master/pf4j/src/main/java/ro/fortsoft/pf4j/AbstractPluginManager.java#L753).
The simple solution is to create a protected method validatePluginDescriptor in AbstractPluginManager and to call this method for each PluginDescripotor instance (no more PluginDescriptorValidator).
What you say?

@coveralls
Copy link

coveralls commented Mar 30, 2017

Coverage Status

Coverage decreased (-0.5%) to 17.397% when pulling 5d3ea84 on cominvent:validator into c6aa1f1 on decebals:master.

@janhoy
Copy link
Member Author

janhoy commented Mar 30, 2017

Agree that this was a more elegant location for the validation. I also had to fix some of the validation tests that previously failed by explicitly calling validation from the test code.
Alternatively we could have created a separate test class for validation of incomplete descriptors.

@decebals
Copy link
Member

I propose you to move the code from PluginDescriptorUtils in AbstractPluginManager and to delete:

  • PluginDescriptorUtils class
  • code that tests validity of PluginDescriptor from *PluginDescriptorFinderTest.java (revert the modifications)

You can add a testValidatePluginDescriptor in AbstractPluginManagerTest. In the future if we see that validatePluginDescriptor method is a candidate for an extension point (many overwrites for this method) we can create an interface PluginDescriptorValidator with a default implementation (DefaultPluginDescriptorValidator) but in this moment I think that it's not necessary to do this (we complicate the library and increase artificially the learning curve ).

Is it OK for you?

@janhoy
Copy link
Member Author

janhoy commented Mar 31, 2017

Think I've accommodated most of it now. Found no AbstractPluginManagerTest so I created a DefaultPluginManagerTest

@coveralls
Copy link

coveralls commented Mar 31, 2017

Coverage Status

Coverage increased (+5.8%) to 23.66% when pulling 391d101 on cominvent:validator into c6aa1f1 on decebals:master.

@decebals decebals merged commit 9f28446 into pf4j:master Apr 1, 2017
@decebals
Copy link
Member

decebals commented Apr 1, 2017

Thanks! Good work.

@janhoy janhoy deleted the validator branch April 1, 2017 08:36
@janhoy
Copy link
Member Author

janhoy commented Apr 1, 2017

Good solution, easy to plug own validation. In my scenario I accept a descriptor without a PluginClass since Solr already has another way of handling that.

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.

3 participants