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

Validation (was @Mandatory annotation : fail if property is not set) #30

Open
mikeChampagne opened this issue Jun 4, 2013 · 25 comments
Assignees
Milestone

Comments

@mikeChampagne
Copy link

Like you wrote in your faq ( http://lviggiano.github.io/owner/#faq ), we might want some properties without a default value, thus forcing the user to set it in the properties file. It would be nice to have the feature juste like you describe it.

@lviggiano
Copy link
Collaborator

👍 tell me if this fits your case:

interface Example extends Config {
    @Mandatory
    // @DefaultValue("") what should happen if the property is just an empty string ?
    String someValue();
} 

@Test
public void testMandatoryProperty() {
    try {
        Example cfg = ConfigFactory.create(Example.class);
        Assert.fail("exception was expected");
    } catch (MandatoryPropertyMissingException ex) {
        // fine
    }
}

When @Mandatory is applied to the class level, then all the properties (without a @DefaultValue) are mandatory. Does it sound good?

@mikeChampagne
Copy link
Author

I think that implementation of @mandatory would work pretty fine. Put it if the user must define it, don't if a null value is acceptable. That's good for me.

If you use testng, take a look at @test(expectedExceptions = MandatoryPropertyMissingException.class) to remove the "try catch block" and the "Assert.fail()" from your test methods ;)

@lviggiano
Copy link
Collaborator

I am thinking that... it may be more nice to have something like:

@Validate(NotNull.class, NotEmpty.class, Positive.class, ...)
Integer minAge();

would be more flexible and it can do the @Mandatory work as well leaving the user to implement his own additional validation logic... what do you think?

JUnit has the @Test(expected=FooException.class) too; test-ng is great but I find that @JUnit does have all I need for most of the times. Maybe test-ng has some multithreaded support that could have saved me some time today for the MultithreadedReloadTest that I showed you by email, but it was a good exercise anyway.

@ffbit
Copy link
Contributor

ffbit commented Jun 5, 2013

May be it worths to implements a kind of fail-fast strategy which tries to load all config's @Mandatory properties during Config instantiation time?

WDYT?

@lviggiano
Copy link
Collaborator

It is exactly what I thought to do: when calling ConfigFactory.create, take the created proxy and for every method declaring a @Validate annotation call the method and apply the validators classes, if any of them throws an exception (with explaining message) then that exception is passed through create() to the user.

Since I am implementing the automatic reload() I think that if a validation error happens during an autoreload then the user doesn't get any exception - create() is done before, and now it's just a "background" reload - and the application continue working without malfunctioning. So probably I'll need to introduce a logging interface and maybe some classes to enable support to Log4j, commons logging, slf4j, and setting java.util.logging as default.

@lviggiano
Copy link
Collaborator

beware that if you declare a primitive type, they will throw NullPointerException if the property is not set or NumberFormatException if the property is set to a non-numeric value:

interface MyConfig extends Config {
    int maxAge(); // suppose this property is not assigned 
} 

MyConfig cfg = ConfigFactory.create(MyConfig.class);
cfg.maxAge(); //will throw a NullPointerException when invoked maxAge()

that's the intended behavior now, but if we introduce a validation process, the validation step can intercept this error and pass it to the user during creation.

for instance:

interface MyConfig extends Config {
    @Validate
    int maxAge(); // suppose this property is not assigned 
} 

MyConfig cfg = ConfigFactory.create(MyConfig.class);  //will throw a ValidationException or something on create()
cfg.maxAge();

Same concept must apply to enums too (and we need to check also other supported types)

@ffbit
Copy link
Contributor

ffbit commented Jun 5, 2013

I agree

@mikeChampagne
Copy link
Author

Yes, I agree that @Validate is better than @Mandatory. You can offer some basic validators, and let users create their own. And how will you associate validators with variable types? I mean, what happens in cases where the validator can't be applied to the field, like this :

@Validate(NotNull.class, NotEmpty.class)
int nbCars();
@Validate(Positive.class)
String garageName();

Throw an exception?

@lviggiano
Copy link
Collaborator

@Validate(NotNull.class, NotEmpty.class)
int nbCars();

And the above can be easily combined to @Validate(Mandatory.class) too as shorthand, which will delegate to NotNull and NotEmpty.

@Validate(Positive.class)
String garageName();
Throw an exception?

Yes, an exception should be thrown here (ValidationException or something)

Type conversion should not be an issue, we'll find the way to handle that in the way the user would expect.

The thing is more complex, since we now support also Collections and Array return types, so it a Validator shall be able to validate simple values or array/collections of values as well. This must be invisible to the user, the API will check if it has to validate a simple object or the array and call the correspondant method. We need to think more in deep about this.

At the moment I think that Validator will have an interface like this:

Validator {
    void validate(Object input) throws ValidationException;
    void validate(Iterable input) throws ValidationException;
}

but it's just a concept to be expanded.

Of course we'll provide some basic validators (I think about an class with several static inner classes atm, or a separate package) and eventually also a BaseValidator to use as an adapter for user-defined return types and checks.

@bbossola
Copy link
Contributor

I know I am late, but why don't you integrate the bean validation specs (JSR 349) for basic validation?
http://beanvalidation.org/1.1/
I am experimenting with the reference implementation (hibernate validator) and it looks quite good

@lviggiano
Copy link
Collaborator

Not late at all, I've not yet started to implement this. I was looking at hibernate validator too, thanks to @ffbit suggestion (http://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#validator-defineconstraints-spec) and it looks what we need here.

@mikeChampagne
Copy link
Author

I'm happy to see your 1.0.4 release :) What about this validators feature? Any work in progress?

@lviggiano
Copy link
Collaborator

not yet, it's something that needs to be properly designed. Yesterday I committed some XML support (not that I am a fan of XML though...), see the example in last comment I posted in issue #5.

I would like to add also YAML support, since it doesn't look any hard to achieve soon.
Also I feel that some pluggable logging is necessary as well as the validation, it's the next big thing I will work on.

The website documentation has been totally rewritten, this took some time but I'm happy with the result.

@mikeChampagne
Copy link
Author

You're right that validation would need logging. As soon as you offer validation by annotations, I will integrate OWNER at work. I like the new website documentation, and great job for this project :)

@lviggiano
Copy link
Collaborator

I can't make promises regarding time for a feature to be ready, since I'm working on this project in my free time, btw I hope it won't take it long.

@lviggiano
Copy link
Collaborator

Hi @mikeChampagne . Today I released version 1.0.5. Have a look at the release notes. There is a feature called "event notification" which allows for some control over how property are changing and allow the user to rollback some unwanted change. I think this is the first stepstone to validation implementation. In fact I think I'll implement the validation annotations over the event notification mechanism.

Thanks for your patience and support.

@lviggiano lviggiano mentioned this issue Oct 18, 2013
@ghost ghost assigned lviggiano Oct 18, 2013
@gunnarmorling
Copy link

Hi, just came across this neat library which I think is very useful. One of my first thoughts was "Couldn't this be integrated with Bean Validation?" (I'm comitting to Hibernate Validator) and apparently others got that idea as well.

Integration actually shouldn't be difficult using the method validation functionality new in Bean Validation 1.1. You'd just have to extend the invocation handler of configuration proxies and invoke the ExecutableValidator API. If the validation yields a constraint violation, you could raise a ConstraintViolationException.

In case you need any help with Bean Validation, I'd be glad to help.

@lviggiano
Copy link
Collaborator

@gunnarmorling thanks! I am planning to work on this. But any help is welcome.

@caillette
Copy link

Hi all,

Thanks to Stack Overflow I found OWNER and I spend my evening reading its documentation, its a very nice library.

Here are some thoughts about validation.

Validating each field independantly is nice for expressing simple constraints but sometimes there are constraints across configuration fields.

@Validate( MyValidator.class )
interface MyConfig extends Config {
  int minAge() ; 
  int maxAge() ;   

  class MyValidator implements ClassValidator< MyConfig > {
    public void validate( MyConfig myConfig ) {
      if( myConfig.minAge() > myConfig.maxAge() ) {
        throw new ValidationException( "minAge > maxAge" ) ;
      }
    }  
  }  
} 

+1 for @Validate(NotNull.class, NotEmpty.class, Positive.class, ...)

+1 for fail-fast of course.

-1 for Validator#validate( Iterable input ). I understand that you want to reuse the single-value validator but you'd better deal with that with implementation classes.

-1 for a direct support of BeanValidation which is a huge spec (I wouldn't say it is overengineered because that's a truly difficult subject). It would be better to let users hook their usage of BeanValidation on OWNER's lightweight validation system.

@lviggiano
Copy link
Collaborator

@caillette thanks for your feedback.

@gunnarmorling
Copy link

Validating each field independantly is nice for expressing simple constraints but sometimes there are constraints across configuration fields.

Bean Validation addresses this requirement via class-level constraints.

-1 for a direct support of BeanValidation which is a huge spec

Personally I don't think there is an advantage in creating a solution from scratch instead of delegating the task of validation to an existing and proven standard. Why bother with de-fining (and implementing) all the required validation semantics and logic when you can basically get it for free? The task may seem trivial in the beginning, but as more features are going to be added (which always is going to happen), complexity will begin to creep in.

Regarding complexity of the BV spec, I wouldn't say it's huge (but then I'm biased as I'm member of the expert group ;-)); The API is very easy to use for the common cases, with enough flexibility to also address more specifc scenarios. Admittedly e.g. group sequences can be a bit hard to digest, but this doesn't affect you at all as long as you don't use it. But if you happen to have requirements for such more advanced features, they are there ready to be used, e.g. also external configuration via XML, I18N-aware message interpolation etc).

Also chances are that people already use Bean Validation in other parts of their application, so they'll be happy if they can use the same semantics also for configuration property validation instead of having to deal with another solution.

@caillette
Copy link

@gunnarmorling By "direct support" I mean: make Bean Validation a mandatory runtime dependency for OWNER. This has plenty of drawbacks, including potential version conflicts.

An indirect support would mean plugging Bean Validation to OWNER's validation system. It makes sense to offer such an option.

The simplest validation system I can think about is an interface with a single validation method validating the whole Configuration object, and returning a set of objects describing what went wrong. This can be seen as a subset of Bean Validation API, so I guess Bean Validation could easily hook on it.

@gunnarmorling
Copy link

make Bean Validation a mandatory runtime dependency for OWNER. This has plenty of drawbacks, including potential version conflicts.

I'd make validation an optional feature. If a Bean Validation provider is present it can be used to validate the configuration, otherwise not. That's how many other techs (e.g. JPA) make use of Bean Validation.

@mbrotz
Copy link

mbrotz commented Oct 29, 2019

Hi. Is there any progress on validation? That would be a great feature!

@lviggiano
Copy link
Collaborator

Nope. I am not working very much on this project; I am working for a private company and I don't have too much time left.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants