Details

    • Estimated Complexity:
      Unknown
    1. patch-jaxrsvalidationinvoker-tests.txt
      23 kB
      Andriy Redko
    2. patch-validation-exception-mapper.txt
      7 kB
      Andriy Redko
    3. patch-validation-paramnameprovider.txt
      16 kB
      Andriy Redko
    4. patch-validation-poc.txt
      32 kB
      Andriy Redko
    5. patch-validation-spring-tests.txt
      11 kB
      Andriy Redko
    6. patch-validation-tests.txt
      5 kB
      Andriy Redko

      Activity

      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      As for a moment, Hibernate Validator 5+ is the only available JSR-349 reference implementation. Unfortunately, Apache BVal still implements the older version, JSR-303.
      Do you see any obstacles integrating Hibernate Validator into Apache CXF as a bean validation provider? The project is quite mature and stable.
      Hopefully, the implementations are interchangeable so Apache BVal could use later on.

      Thank you.

      Best Regards,
      Andriy Redko

      Show
      Andriy Redko added a comment - Hi Sergey, As for a moment, Hibernate Validator 5+ is the only available JSR-349 reference implementation. Unfortunately, Apache BVal still implements the older version, JSR-303. Do you see any obstacles integrating Hibernate Validator into Apache CXF as a bean validation provider? The project is quite mature and stable. Hopefully, the implementations are interchangeable so Apache BVal could use later on. Thank you. Best Regards, Andriy Redko
      Hide
      Sergey Beryozkin added a comment -

      Hi Andriy

      Hibernate Validator has an Apache 2.0 license so that is good; generally speaking, I do not see a problem because we'll have an optional Maven BeanValidation provider dependency, so you are right, users would be able to use a specific implementation that suits best.

      Can you please check JAX-RS 2.0 Chapter 7 and see if JSR-349 specific annotations or rules are mentioned there ? If yes then I guess it will definitely have to be a Hibernate provider for now

      Thanks, Sergey

      Show
      Sergey Beryozkin added a comment - Hi Andriy Hibernate Validator has an Apache 2.0 license so that is good; generally speaking, I do not see a problem because we'll have an optional Maven BeanValidation provider dependency, so you are right, users would be able to use a specific implementation that suits best. Can you please check JAX-RS 2.0 Chapter 7 and see if JSR-349 specific annotations or rules are mentioned there ? If yes then I guess it will definitely have to be a Hibernate provider for now Thanks, Sergey
      Hide
      Sergey Beryozkin added a comment -

      According to http://beanvalidation.org/1.1/, what is new there is a method level validation which is what JAX-RS 2.0 requires from the stacks implementing its chapter 7.

      That said, apparently, Apache BVal allows for a method-level validation too:
      http://bval.apache.org/obtaining-a-validator.html

      which seems to depend on Guice...

      To be honest I'd prefer the simplest option possible , as far as the integration is concerned. If this is what Hibernate Validator can offer then it probably makes sense to start from using it to support the integration for now. The other thing is that JAX-RS 2.0 talks about JSR-339, so I guess for CXF to claim it supports an optional JAX-RS 2.0 BeanValidation then it has to be Hibernate given that it implements it.

      So I guess at the moment I'd vote for using a Hibernate. Ideally though we would write it such, the CXF validation layer, that if users will want to use Apache BVal, then it would just work by getting it installed

      Show
      Sergey Beryozkin added a comment - According to http://beanvalidation.org/1.1/ , what is new there is a method level validation which is what JAX-RS 2.0 requires from the stacks implementing its chapter 7. That said, apparently, Apache BVal allows for a method-level validation too: http://bval.apache.org/obtaining-a-validator.html which seems to depend on Guice... To be honest I'd prefer the simplest option possible , as far as the integration is concerned. If this is what Hibernate Validator can offer then it probably makes sense to start from using it to support the integration for now. The other thing is that JAX-RS 2.0 talks about JSR-339, so I guess for CXF to claim it supports an optional JAX-RS 2.0 BeanValidation then it has to be Hibernate given that it implements it. So I guess at the moment I'd vote for using a Hibernate. Ideally though we would write it such, the CXF validation layer, that if users will want to use Apache BVal, then it would just work by getting it installed
      Hide
      Andriy Redko added a comment -

      Definitely agree with you. JAX-RS 2.0 claims to support JSR-339 so it seems like Hibernate Validator is the right choice.

      Also, no doubts that validation layer should be shielded off from particular implementation. From my experience working with Bean Validation, JSRs are quire well designed in order to provide interfaces and minimize the actual wiring of implementation to couple of places.

      Show
      Andriy Redko added a comment - Definitely agree with you. JAX-RS 2.0 claims to support JSR-339 so it seems like Hibernate Validator is the right choice. Also, no doubts that validation layer should be shielded off from particular implementation. From my experience working with Bean Validation, JSRs are quire well designed in order to provide interfaces and minimize the actual wiring of implementation to couple of places.
      Hide
      Daniel Kulp added a comment -

      I know the Hibernate Validator specifically says it's Apache licensed. However, does it rely on the hibernate libraries themselves which are LGPL? That would certainly cause an issue.

      Basically, please double check that everything we need to provide the validation is really Apache licensed.

      Show
      Daniel Kulp added a comment - I know the Hibernate Validator specifically says it's Apache licensed. However, does it rely on the hibernate libraries themselves which are LGPL? That would certainly cause an issue. Basically, please double check that everything we need to provide the validation is really Apache licensed.
      Hide
      Andriy Redko added a comment -

      Will do, thanks.

      Show
      Andriy Redko added a comment - Will do, thanks.
      Hide
      Andriy Redko added a comment -

      So far following dependencies are explicitly required by Hibernate Validator:

      Both project are licensed under Apache License 2.0.
      Thanks a lot for pointing out this important issue.

      Show
      Andriy Redko added a comment - So far following dependencies are explicitly required by Hibernate Validator: jboss-logging ( https://github.com/jboss-logging/jboss-logging/ , relicensed from LGPL to Apache License 2.0 a year ago) classmate ( https://github.com/cowtowncoder/java-classmate ) Both project are licensed under Apache License 2.0. Thanks a lot for pointing out this important issue.
      Hide
      Sergey Beryozkin added a comment -

      Hi Andriy, thanks for checking it.

      I think we'd probably need to introduce another module eventually, say, cxf-rt-rs-bean-validator, however for now lets start with adding a package like "org.apache.cxf.jaxrs.validation" to the existing cxf-rt-frontend-jaxrs module and do the integration code there.

      Perhaps you can consider doing a sequence of patches given that the whole effort may take some time. For example, how about supporting the very basic example first with say @NonNull annotations attached to JAX-RS method parameters ? Next I can contribute from my end a basic JAX-RS ExceptionMapper implementation which catches validation exceptions as per JAX-RS 2.0 chapter 7 and we will have BeanValidation POC done

      At the next stage, we can push some of the common code to CXF core module so that JAX-WS frontend can also get the bean validation supported.
      Thanks for looking into it, cheers.

      Show
      Sergey Beryozkin added a comment - Hi Andriy, thanks for checking it. I think we'd probably need to introduce another module eventually, say, cxf-rt-rs-bean-validator, however for now lets start with adding a package like "org.apache.cxf.jaxrs.validation" to the existing cxf-rt-frontend-jaxrs module and do the integration code there. Perhaps you can consider doing a sequence of patches given that the whole effort may take some time. For example, how about supporting the very basic example first with say @NonNull annotations attached to JAX-RS method parameters ? Next I can contribute from my end a basic JAX-RS ExceptionMapper implementation which catches validation exceptions as per JAX-RS 2.0 chapter 7 and we will have BeanValidation POC done At the next stage, we can push some of the common code to CXF core module so that JAX-WS frontend can also get the bean validation supported. Thanks for looking into it, cheers.
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      Sounds very reasonable and aligned with my own view on that. Let's start simple.
      The one thing which I would like to consider in this scope is integrating validation provider with Spring bus (if you think it make sense) so the validation provider will be available for injection.
      I (and many other developers) often use Apache CXF and Spring together and I see a great benefit from that. I don't expect it to be complicated but carefully thought out. If implementation turns out not to be straightforward, I will postpone integration with Spring.

      Thank you.

      Show
      Andriy Redko added a comment - Hi Sergey, Sounds very reasonable and aligned with my own view on that. Let's start simple. The one thing which I would like to consider in this scope is integrating validation provider with Spring bus (if you think it make sense) so the validation provider will be available for injection. I (and many other developers) often use Apache CXF and Spring together and I see a great benefit from that. I don't expect it to be complicated but carefully thought out. If implementation turns out not to be straightforward, I will postpone integration with Spring. Thank you.
      Hide
      Sergey Beryozkin added a comment - - edited

      Hi, try a custom invoker:

      http://cxf.apache.org/docs/jax-rs-filters.html#JAX-RSFilters-Custominvokers

      This is where you will get all the type info, and do the validation. Custom invokers can be used with and without Spring, and we will need a non-Spring validation support as well, so it will work for all users. Similar idea will also work for JAX-WS.

      Or may be you can implement the validation in the input CXF interceptor, PRE-INVOKE phase, all the invocation parameters are already available there,

      List<Object> methodParams = MessageContentsList getContentsList(m); 
      

      JAX-RS target method is available as "org.apache.cxf.resource.method" property. JAX-RS OperationResourceInfo class (with the access to all of JAX-RS specific info for the current method and its class owner) is available from "message.getExchange().get(OperationResourceInfo.class)". Using a CXF interceptor may make it simpler to reuse some of the code with the JAX-WS runtime at the next stage
      Interceptors can also be used with/without Spring: example, an interceptor can be registered on the bus or the individual endpoints

      Sergey

      Show
      Sergey Beryozkin added a comment - - edited Hi, try a custom invoker: http://cxf.apache.org/docs/jax-rs-filters.html#JAX-RSFilters-Custominvokers This is where you will get all the type info, and do the validation. Custom invokers can be used with and without Spring, and we will need a non-Spring validation support as well, so it will work for all users. Similar idea will also work for JAX-WS. Or may be you can implement the validation in the input CXF interceptor, PRE-INVOKE phase, all the invocation parameters are already available there, List< Object > methodParams = MessageContentsList getContentsList(m); JAX-RS target method is available as "org.apache.cxf.resource.method" property. JAX-RS OperationResourceInfo class (with the access to all of JAX-RS specific info for the current method and its class owner) is available from "message.getExchange().get(OperationResourceInfo.class)". Using a CXF interceptor may make it simpler to reuse some of the code with the JAX-WS runtime at the next stage Interceptors can also be used with/without Spring: example, an interceptor can be registered on the bus or the individual endpoints Sergey
      Hide
      Sergey Beryozkin added a comment -

      Apache BVal has a 1.1 branch, good news:
      https://svn.apache.org/repos/asf/bval/branches/bval-11/

      Lets do an initial implementation with Hibernate (with an optional Maven dep) and then check how Apache BVal can be used alternatively, may be we can ask Romain to advise us on that...

      Show
      Sergey Beryozkin added a comment - Apache BVal has a 1.1 branch, good news: https://svn.apache.org/repos/asf/bval/branches/bval-11/ Lets do an initial implementation with Hibernate (with an optional Maven dep) and then check how Apache BVal can be used alternatively, may be we can ask Romain to advise us on that...
      Hide
      Andriy Redko added a comment -

      Will follow the implementation as per comments, thanks!

      Show
      Andriy Redko added a comment - Will follow the implementation as per comments, thanks!
      Hide
      Andriy Redko added a comment - - edited

      Hi Sergey,

      I looked through implementation options and would like to try with interceptor approach first. It seems like natural fit and lightweight from implementation prospective. Custom Invoker would be a second / fallback option. My explanation for that: custom invoker gives much more control and insights (not sure we need that, at least now) BUT as far as I know, there could be only one custom invoker in a flow and people do override it/inherit from it (I saw couple of examples). Interceptor is pluggable and much easier to understand.

      What do you think?
      Thanks.

      Andriy

      Show
      Andriy Redko added a comment - - edited Hi Sergey, I looked through implementation options and would like to try with interceptor approach first. It seems like natural fit and lightweight from implementation prospective. Custom Invoker would be a second / fallback option. My explanation for that: custom invoker gives much more control and insights (not sure we need that, at least now) BUT as far as I know, there could be only one custom invoker in a flow and people do override it/inherit from it (I saw couple of examples). Interceptor is pluggable and much easier to understand. What do you think? Thanks. Andriy
      Hide
      Sergey Beryozkin added a comment -

      Hi, using an interceptor sounds reasonable.

      The question is when/how to do the initial introspection of Bean Validation annotations, doing it on every request inside the interceptor won't be efficient, but I think we need to make an initial/basic POC done make as if it will be the case, the interceptor initiates the introspection. At the next stage we will think on how to optimize it, such that we can still have an optional dependency on Bean Validation API

      Cheers, Sergey

      Show
      Sergey Beryozkin added a comment - Hi, using an interceptor sounds reasonable. The question is when/how to do the initial introspection of Bean Validation annotations, doing it on every request inside the interceptor won't be efficient, but I think we need to make an initial/basic POC done make as if it will be the case, the interceptor initiates the introspection. At the next stage we will think on how to optimize it, such that we can still have an optional dependency on Bean Validation API Cheers, Sergey
      Hide
      Andriy Redko added a comment -

      JSR-349 Bean Validation 1.1 PoC.

      Show
      Andriy Redko added a comment - JSR-349 Bean Validation 1.1 PoC.
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      I have done a small but functional enough PoC for adding JSR-349 to Apache CXF. It covers method parameters validation only and returns 400 / Bad Request if validation doesn't pass (JSR-349, Chapter 7).

      The one issue I am concerning about right now is required dependency to Java EL implementation. I have added Java EL 3.0 implementation from Glassfish (license is CDDL, https://glassfish.java.net/public/CDDL+GPL_1_1.html) but I have doubts that license is suitable to us. I have checked Apache BVal and they use implementation from Tomcat project (jasper-el) though it's no 3.0 but older one.

      Feedback is very appreciated.
      Thanks.

      Andriy

      Show
      Andriy Redko added a comment - Hi Sergey, I have done a small but functional enough PoC for adding JSR-349 to Apache CXF. It covers method parameters validation only and returns 400 / Bad Request if validation doesn't pass (JSR-349, Chapter 7). The one issue I am concerning about right now is required dependency to Java EL implementation. I have added Java EL 3.0 implementation from Glassfish (license is CDDL, https://glassfish.java.net/public/CDDL+GPL_1_1.html ) but I have doubts that license is suitable to us. I have checked Apache BVal and they use implementation from Tomcat project (jasper-el) though it's no 3.0 but older one. Feedback is very appreciated. Thanks. Andriy
      Hide
      Sergey Beryozkin added a comment -

      Hi Andriy

      This is impressive, thanks for providing a patch so fast.
      I think that CDDL might be OK, all javax.* artifacts seem to have a CDDL license.

      I'm going to work tomorrow on providing the more complete feedback. We'd probably need to make EL dependencies optional anyway, and we'd need to figure out how to make it optional for the RS frontend, but I'm happy we already have something to experiment with

      Cheers, Sergey

      Show
      Sergey Beryozkin added a comment - Hi Andriy This is impressive, thanks for providing a patch so fast. I think that CDDL might be OK, all javax.* artifacts seem to have a CDDL license. I'm going to work tomorrow on providing the more complete feedback. We'd probably need to make EL dependencies optional anyway, and we'd need to figure out how to make it optional for the RS frontend, but I'm happy we already have something to experiment with Cheers, Sergey
      Hide
      Andriy Redko added a comment -

      Thanks a lot! I will eagerly wait for your feedback.

      A small note, I've declared all implementation dependencies as "optional" but all API dependencies as non-optional.
      Also, I found a test scaffolding to be tough to use so instead of end-to-end testing I've added interceptor's part only.

      Thanks!
      Andriy

      Show
      Andriy Redko added a comment - Thanks a lot! I will eagerly wait for your feedback. A small note, I've declared all implementation dependencies as "optional" but all API dependencies as non-optional. Also, I found a test scaffolding to be tough to use so instead of end-to-end testing I've added interceptor's part only. Thanks! Andriy
      Hide
      Sergey Beryozkin added a comment -

      Hi, here are some more comments; I need to finish some pending work and then I will join but in meantime please try to do/experiment with the following:

      • ValidationInInterceptor: I wonder if it was correct to start with interceptors after all; we can definitely make the code getting a Method neutral between the various frontends but the issue is that an actual service instance is really usually available at the invoker level; For example, in this case, if we have a per-request JAX-RS instance then asking a resource provider to get us an instance will lead to JAXRSInvoker creating a duplicate instance later.
        JAXRSInvoker may also inject path/form/etc parameters into service objects if it is per-request life-cycle, so the validation would miss those parameters if we do it early.

      I guess similarly is the case with various JAX-WS invokers, lets keep it as is for now, but we will need to revisit this issue and I feel that may be the invoker will need to be done in the end - it is as pluggable as interceptors are.

      • Please add ValidationOutInterceptor, method return values have to be validated too. May be we will end up with the using the invoker in the end, but lets continue with the interceptors for now, I can do the invoker code myself when I work on applying the patch
      • Can you experiment with ValidationProvider caching the introspection info ? I'm not exactly sure what form it will take in the end but for now please have ValidationInInterceptor & ValidationOutInterceptor injected with ValidationProvider instead of creating it in ValidationInInterceptor, and may be have a sync block in ValidationProvider which will do the introspection of BeanValidation metadata / EL expressions if it is not there and then use it;
        May be we will do it later at the endpoint creation time to avoid the synchronization, but for now lets try the POC way
      • Right now I thinking that the initial step for JAX-RS at least is that users will need to register validation exception mappers and invoker (or interceptors) manually, in order to avoid the compile deps on the validation API - we will have to revisit this issue later; or may be it will end up in the other module, will depend if we want CXF to ship the API deps, to be discussed

      I guess this is it for now; hope the above sounds reasonable. I'm going to help ASAP, need to resolve few pending work items first, thanks

      Show
      Sergey Beryozkin added a comment - Hi, here are some more comments; I need to finish some pending work and then I will join but in meantime please try to do/experiment with the following: ValidationInInterceptor: I wonder if it was correct to start with interceptors after all; we can definitely make the code getting a Method neutral between the various frontends but the issue is that an actual service instance is really usually available at the invoker level; For example, in this case, if we have a per-request JAX-RS instance then asking a resource provider to get us an instance will lead to JAXRSInvoker creating a duplicate instance later. JAXRSInvoker may also inject path/form/etc parameters into service objects if it is per-request life-cycle, so the validation would miss those parameters if we do it early. I guess similarly is the case with various JAX-WS invokers, lets keep it as is for now, but we will need to revisit this issue and I feel that may be the invoker will need to be done in the end - it is as pluggable as interceptors are. Please add ValidationOutInterceptor, method return values have to be validated too. May be we will end up with the using the invoker in the end, but lets continue with the interceptors for now, I can do the invoker code myself when I work on applying the patch Can you experiment with ValidationProvider caching the introspection info ? I'm not exactly sure what form it will take in the end but for now please have ValidationInInterceptor & ValidationOutInterceptor injected with ValidationProvider instead of creating it in ValidationInInterceptor, and may be have a sync block in ValidationProvider which will do the introspection of BeanValidation metadata / EL expressions if it is not there and then use it; May be we will do it later at the endpoint creation time to avoid the synchronization, but for now lets try the POC way Right now I thinking that the initial step for JAX-RS at least is that users will need to register validation exception mappers and invoker (or interceptors) manually, in order to avoid the compile deps on the validation API - we will have to revisit this issue later; or may be it will end up in the other module, will depend if we want CXF to ship the API deps, to be discussed I guess this is it for now; hope the above sounds reasonable. I'm going to help ASAP, need to resolve few pending work items first, thanks
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      Thanks a lot, it sounds reasonable. I will try to address your comments during this week. With respect to interceptor vs invoker, I totally rely on your recommendations. I believe you see pros and cons better then me at this point.

      It would be extremely helpful if you guys have a good example of end-to-end JUnit test. At this moment, I do have a test project to verify. f.e. @DefaltValue annotations are being injected before interceptor's call so validation passes even if parameters is not specified but default value is.

      Thanks.
      Andriy.

      Show
      Andriy Redko added a comment - Hi Sergey, Thanks a lot, it sounds reasonable. I will try to address your comments during this week. With respect to interceptor vs invoker, I totally rely on your recommendations. I believe you see pros and cons better then me at this point. It would be extremely helpful if you guys have a good example of end-to-end JUnit test. At this moment, I do have a test project to verify. f.e. @DefaltValue annotations are being injected before interceptor's call so validation passes even if parameters is not specified but default value is. Thanks. Andriy.
      Hide
      Sergey Beryozkin added a comment - - edited

      Sure, you can copy & paste say
      http://svn.apache.org/repos/asf/cxf/trunk/systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JAXRSClientServerStreamingTest.java

      into

      JAXRSClientServerValidationTest.java, with run() having only

      protected void run() {
                  JAXRSServerFactoryBean sf = new JAXRSServerFactoryBean();
                  // replace BookStore.class ref with the one from your test
                  sf.setResourceClasses(BookStore.class);
                  sf.setResourceProvider(BookStore.class,
                                         new SingletonResourceProvider(new BookStore()));
                  sf.setAddress("http://localhost:" + PORT + "/");
                  // add in and out validation interceptors
      
                  sf.create();
      
              }
      

      and then start using WebClient or proxy-based API to stress individual server resource methods.

      So may be you can move your current test into systests/jaxrs ?

      (as a side note - we can probably support client-side Bean validation in time too which would be cool)

      thanks, Sergey

      Show
      Sergey Beryozkin added a comment - - edited Sure, you can copy & paste say http://svn.apache.org/repos/asf/cxf/trunk/systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JAXRSClientServerStreamingTest.java into JAXRSClientServerValidationTest.java, with run() having only protected void run() { JAXRSServerFactoryBean sf = new JAXRSServerFactoryBean(); // replace BookStore.class ref with the one from your test sf.setResourceClasses(BookStore.class); sf.setResourceProvider(BookStore.class, new SingletonResourceProvider( new BookStore())); sf.setAddress( "http: //localhost:" + PORT + "/" ); // add in and out validation interceptors sf.create(); } and then start using WebClient or proxy-based API to stress individual server resource methods. So may be you can move your current test into systests/jaxrs ? (as a side note - we can probably support client-side Bean validation in time too which would be cool) thanks, Sergey
      Hide
      Sergey Beryozkin added a comment - - edited

      We might have both interceptors and the invoker options supported. The former option can help with introducing Validation Feature and also work in the mainstream case, i.e, where it is known that a service object is a singleton, we can throw ISE if it is not a singleton. The invoker will cover all other service lifecycles.

      Show
      Sergey Beryozkin added a comment - - edited We might have both interceptors and the invoker options supported. The former option can help with introducing Validation Feature and also work in the mainstream case, i.e, where it is known that a service object is a singleton, we can throw ISE if it is not a singleton. The invoker will cover all other service lifecycles.
      Hide
      Andriy Redko added a comment -

      Got your point, has sense. I will move my test cases to systest, having good coverage here we could experiment with invoker as well. Thank you.

      Andriy

      Show
      Andriy Redko added a comment - Got your point, has sense. I will move my test cases to systest, having good coverage here we could experiment with invoker as well. Thank you. Andriy
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      Please take a look on the new PoC patch (as time permits) with your major comments addressed:

      • ValidationOutInterceptor has been added and performs return values validation
      • both in- and out-interceptors should be registered manually
      • validation exception mapper should be registered manually
      • exception mapper can distinguish parameters validation (400) and return values validation (500), as per spec's Chapter 7
      • all tests have been moved to systest / jaxrs

      I am looking into caching validation metadata so not to perform introspection all the time.

      Feedback is very appreciated. Thanks!
      Andriy.

      Show
      Andriy Redko added a comment - Hi Sergey, Please take a look on the new PoC patch (as time permits) with your major comments addressed: ValidationOutInterceptor has been added and performs return values validation both in- and out-interceptors should be registered manually validation exception mapper should be registered manually exception mapper can distinguish parameters validation (400) and return values validation (500), as per spec's Chapter 7 all tests have been moved to systest / jaxrs I am looking into caching validation metadata so not to perform introspection all the time. Feedback is very appreciated. Thanks! Andriy.
      Hide
      Sergey Beryozkin added a comment -

      Hi Andriy

      I think it is looking really good, thanks for spending your time on it.

      IMHO there's enough material now for me to start working on applying the patch. I may not be able to do it this week as I'm taking few days off later in the week but I'll do it asap. In meantime please experiment with caching the metadata, when you get a chance.

      FYI, I'm thinking of doing a couple of minor tweaks, one of them:

      • In CXF we can figure out where the exception came from based on the analysis of the current message, example, if PhaseInterceptorChain.getCurrentMessage().getExchange().getOutMessage() is null then the exception came from the inbound chain, otherwise from the outbound one, thus it can help us avoid introducing a pair of CXF specific validation exceptions. This is a very minor simplification really.

      It all looks fine, we can now see how it all can be wired in, nice work, thanks

      Show
      Sergey Beryozkin added a comment - Hi Andriy I think it is looking really good, thanks for spending your time on it. IMHO there's enough material now for me to start working on applying the patch. I may not be able to do it this week as I'm taking few days off later in the week but I'll do it asap. In meantime please experiment with caching the metadata, when you get a chance. FYI, I'm thinking of doing a couple of minor tweaks, one of them: In CXF we can figure out where the exception came from based on the analysis of the current message, example, if PhaseInterceptorChain.getCurrentMessage().getExchange().getOutMessage() is null then the exception came from the inbound chain, otherwise from the outbound one, thus it can help us avoid introducing a pair of CXF specific validation exceptions. This is a very minor simplification really. It all looks fine, we can now see how it all can be wired in, nice work, thanks
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      Thanks a lot! I will continue experimenting with caching the metadata. With respect to PhaseInterceptorChain.getCurrentMessage().getExchange().getOutMessage(), I think it's a great idea. It might be a bit unclear for developers to figure out this trick if someone decides to provide own exception wrapper. I think it could be a case as spec's Chapter 7 outlines HTTP error codes but not content (which could be application-specific - text / JSON / XML / ... ). Anyway, I would prefer to go with PhaseInterceptorChain.getCurrentMessage().getExchange().getOutMessage(), it's more lightweight then exceptions hierarchy.

      Thank you.
      Andriy.

      Show
      Andriy Redko added a comment - Hi Sergey, Thanks a lot! I will continue experimenting with caching the metadata. With respect to PhaseInterceptorChain.getCurrentMessage().getExchange().getOutMessage(), I think it's a great idea. It might be a bit unclear for developers to figure out this trick if someone decides to provide own exception wrapper. I think it could be a case as spec's Chapter 7 outlines HTTP error codes but not content (which could be application-specific - text / JSON / XML / ... ). Anyway, I would prefer to go with PhaseInterceptorChain.getCurrentMessage().getExchange().getOutMessage(), it's more lightweight then exceptions hierarchy. Thank you. Andriy.
      Hide
      Sergey Beryozkin added a comment -

      Hi Andriy,
      you have a good point though about the fact that the custom ConstraintValidationException mappers may find it simpler to check for the specific exception type as opposed to going a more low-level check involving the current message. And yes, we have a case for JAX-RS 2.1 enhancement request, i.e, no portable way at the moment to figure in the custom mapper whether it was a in or out validation issue.
      I'd then drop CXF ServerConstraintValidationException for now and default ConstraintValidationException to signal in validation issues, and keep ClientConstraintValidationException, but rename it to ResponseConstraintValidationException; when we do a CXF specific client proxy validation support we can reuse ResponseConstraintValidationException as well. I can take care of this updates next week
      Cheers, Sergey

      Show
      Sergey Beryozkin added a comment - Hi Andriy, you have a good point though about the fact that the custom ConstraintValidationException mappers may find it simpler to check for the specific exception type as opposed to going a more low-level check involving the current message. And yes, we have a case for JAX-RS 2.1 enhancement request, i.e, no portable way at the moment to figure in the custom mapper whether it was a in or out validation issue. I'd then drop CXF ServerConstraintValidationException for now and default ConstraintValidationException to signal in validation issues, and keep ClientConstraintValidationException, but rename it to ResponseConstraintValidationException; when we do a CXF specific client proxy validation support we can reuse ResponseConstraintValidationException as well. I can take care of this updates next week Cheers, Sergey
      Hide
      Andriy Redko added a comment -

      Hi Sergey! Sounds great, thanks!
      Andriy.

      Show
      Andriy Redko added a comment - Hi Sergey! Sounds great, thanks! Andriy.
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      I've looked deeply into Hibernate Validator internals and can confirm the fact that Hibernate Validator does use caching quite extensively for classes (beans), its members and methods. The lookup for respective method / class validation metadata is being done once and is stored inside internal cache (which is Hibernate's internal thread-safe ConcurrentReferenceHashMap). The bean introspection (over reflection) is also done only once.

      With respect to current implementation, we probably should refactor ValidationProvider to have ValidatorFactory member declared as static so to create it only once (right now we have two instances, one for In-interceptor and one for out-interceptor). Please let me know if you need any help from my side, I would prefer not to submit another patch right now to mess up your work unless you are OK with that.

      Thanks.
      Andriy.

      Show
      Andriy Redko added a comment - Hi Sergey, I've looked deeply into Hibernate Validator internals and can confirm the fact that Hibernate Validator does use caching quite extensively for classes (beans), its members and methods. The lookup for respective method / class validation metadata is being done once and is stored inside internal cache (which is Hibernate's internal thread-safe ConcurrentReferenceHashMap). The bean introspection (over reflection) is also done only once. With respect to current implementation, we probably should refactor ValidationProvider to have ValidatorFactory member declared as static so to create it only once (right now we have two instances, one for In-interceptor and one for out-interceptor). Please let me know if you need any help from my side, I would prefer not to submit another patch right now to mess up your work unless you are OK with that. Thanks. Andriy.
      Hide
      Sergey Beryozkin added a comment - - edited

      Hi Andriy,

      I've applied your patch with few modifications. For the most part it's been really to do with pushing as much common code as possible to the core module so that a relevant JAXWS implementation can extend that and do few minor overrides:

      http://svn.apache.org/r1537558

      Here is the main changes:

      • ValidationProvider has few more constructors and was moved to the core
      • Abstract interceptors have been introduced with JAXRS in/out interceptors doing minor overrides
      • JAXRSInvoker and feature have been added
      • Out interceptor can be injected with the same instance of ValidationProvider or in case of the default instantiation it will pick it up from the exchange
      • Validation API dep is currently optional - we need to have an OSGI-friendly API spec bundle ready first before we can make this dependency non-optional.
      • You will note that all other dependencies are in systest/jaxrs - this is because javax.el build has been 'messed up' a bit with the target restricted to Java 7 which is a problem for CXF because it supports Java 6. It's not a major problem, I think we can ship for example a Karaf validation feature with the latest javax.el and document it is only available for 1.7 - that should do because the feature is optional.

      Let me know what you think about the code on the trunk, and then we can discuss what else can be done, thanks

      Show
      Sergey Beryozkin added a comment - - edited Hi Andriy, I've applied your patch with few modifications. For the most part it's been really to do with pushing as much common code as possible to the core module so that a relevant JAXWS implementation can extend that and do few minor overrides: http://svn.apache.org/r1537558 Here is the main changes: ValidationProvider has few more constructors and was moved to the core Abstract interceptors have been introduced with JAXRS in/out interceptors doing minor overrides JAXRSInvoker and feature have been added Out interceptor can be injected with the same instance of ValidationProvider or in case of the default instantiation it will pick it up from the exchange Validation API dep is currently optional - we need to have an OSGI-friendly API spec bundle ready first before we can make this dependency non-optional. You will note that all other dependencies are in systest/jaxrs - this is because javax.el build has been 'messed up' a bit with the target restricted to Java 7 which is a problem for CXF because it supports Java 6. It's not a major problem, I think we can ship for example a Karaf validation feature with the latest javax.el and document it is only available for 1.7 - that should do because the feature is optional. Let me know what you think about the code on the trunk, and then we can discuss what else can be done, thanks
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      I reviewed your changes and they are look very good to me. Thank you.
      As for PoC feature-completeness, from my standpoint, would be nice to have:

      • research / preserve argument names for violated constraints (if you look right now on violation details, the method argument names are lost, f.e. 'addBook.arg1' instead of 'addBook.name')
      • allow injection of validation provider (if available) into Spring context (when Spring bus is used)

      What do you think?
      Thank you.

      Andriy

      Show
      Andriy Redko added a comment - Hi Sergey, I reviewed your changes and they are look very good to me. Thank you. As for PoC feature-completeness, from my standpoint, would be nice to have: research / preserve argument names for violated constraints (if you look right now on violation details, the method argument names are lost, f.e. 'addBook.arg1' instead of 'addBook.name') allow injection of validation provider (if available) into Spring context (when Spring bus is used) What do you think? Thank you. Andriy
      Hide
      Sergey Beryozkin added a comment - - edited

      Hi Andriy,

      Thanks. By the way, I forgot to clarify that I updated the interceptors to throw the exceptions if they could not do their work, as the application code will actually depend on them and avoid the manual validation.

      Good point about preserving the arg names, I saw the log messages.

      Not sure about the spring injection, I think it is already covered. AbstractValidatingInterceptor has a public setter accepting ValidationProvider; users can also inject it via a property, say in jaxrs:properties. The default initialization will only be needed if the provider has not been injected. In most cases I'd expect both in & out interceptors sharing the same provider instance injected into both of them via Spring/etc.

      Can you also please play with with the case where we have a JAX-RS root resource class extending an abstract class or implementing an interface, with JAX-RS annotations being on the implementation class only and Bean Validation annotations being in the abstract class or interface. Chapter 7 mentions cases like this (in context of the annotation inheritance), I wonder whether CXF needs to do something about it or Hibernate will discover the inherited annotations itself.

      Thanks, Sergey

      Show
      Sergey Beryozkin added a comment - - edited Hi Andriy, Thanks. By the way, I forgot to clarify that I updated the interceptors to throw the exceptions if they could not do their work, as the application code will actually depend on them and avoid the manual validation. Good point about preserving the arg names, I saw the log messages. Not sure about the spring injection, I think it is already covered. AbstractValidatingInterceptor has a public setter accepting ValidationProvider; users can also inject it via a property, say in jaxrs:properties. The default initialization will only be needed if the provider has not been injected. In most cases I'd expect both in & out interceptors sharing the same provider instance injected into both of them via Spring/etc. Can you also please play with with the case where we have a JAX-RS root resource class extending an abstract class or implementing an interface, with JAX-RS annotations being on the implementation class only and Bean Validation annotations being in the abstract class or interface. Chapter 7 mentions cases like this (in context of the annotation inheritance), I wonder whether CXF needs to do something about it or Hibernate will discover the inherited annotations itself. Thanks, Sergey
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      Interesting point. I will check the validation annotation inheritance on interfaces / abstract classes.
      Thanks.

      Andriy

      Show
      Andriy Redko added a comment - Hi Sergey, Interesting point. I will check the validation annotation inheritance on interfaces / abstract classes. Thanks. Andriy
      Hide
      Andriy Redko added a comment -

      Hey Sergey,

      I have refactored a little bit test cases in order to inherit basic JAX-RS service from one abstract class with validation constraints and one interface with validation constraints. Hibernate Validator resolves them perfectly (patch-validation-tests.txt).

      Thanks.
      Andriy

      Show
      Andriy Redko added a comment - Hey Sergey, I have refactored a little bit test cases in order to inherit basic JAX-RS service from one abstract class with validation constraints and one interface with validation constraints. Hibernate Validator resolves them perfectly (patch-validation-tests.txt). Thanks. Andriy
      Hide
      Sergey Beryozkin added a comment -

      Thanks Andriy, I'll apply it shortly.

      By the way. How do we validate return values wrapped in JAX-RS Response ? I've updated the out interceptor to actually pass Response.getEntity() to the validator if the original response value is of type Response. would it confuse the validator though, it will know from Method that the actual type is JAX-RS Response. Something to think about

      Thanks, Sergey

      Show
      Sergey Beryozkin added a comment - Thanks Andriy, I'll apply it shortly. By the way. How do we validate return values wrapped in JAX-RS Response ? I've updated the out interceptor to actually pass Response.getEntity() to the validator if the original response value is of type Response. would it confuse the validator though, it will know from Method that the actual type is JAX-RS Response. Something to think about Thanks, Sergey
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      That's an excellent question. I guess Response.getEntity() could be an option if method's return value is marked as @Valid, but it also means we have to introspect in order to make a decision (duplicating the validator's logic). I will try to find an answer in specification. Thanks a lot for bringing it up.

      Andriy.

      Show
      Andriy Redko added a comment - Hi Sergey, That's an excellent question. I guess Response.getEntity() could be an option if method's return value is marked as @Valid, but it also means we have to introspect in order to make a decision (duplicating the validator's logic). I will try to find an answer in specification. Thanks a lot for bringing it up. Andriy.
      Hide
      Sergey Beryozkin added a comment -

      2nd patch has been applied, thanks

      Show
      Sergey Beryozkin added a comment - 2nd patch has been applied, thanks
      Hide
      Andriy Redko added a comment -

      Thanks a lot. I also looked for clarification of generic Response validation inside JSR-339 but found nothing. Not sure we have to validate Response.getEntity() though. I would vote for doing no additional work but delegating Response as-is for validation (which will do nothing right now). What I roughly thought about is to have Response implementation with validation annotations and, by adding f.e. method validated() to ResponseBuilder, return this implementation which naturally will be handed off to validator and processed properly (if @Valid annotation is present). What do you think?

      Thanks a lot.
      Andriy.

      Show
      Andriy Redko added a comment - Thanks a lot. I also looked for clarification of generic Response validation inside JSR-339 but found nothing. Not sure we have to validate Response.getEntity() though. I would vote for doing no additional work but delegating Response as-is for validation (which will do nothing right now). What I roughly thought about is to have Response implementation with validation annotations and, by adding f.e. method validated() to ResponseBuilder, return this implementation which naturally will be handed off to validator and processed properly (if @Valid annotation is present). What do you think? Thanks a lot. Andriy.
      Hide
      Sergey Beryozkin added a comment -

      Updating ResponseBuilder is not realistic. I've actually been poking around it too right now, committing shortly. The current solution I came up with is to validate a single bean only if we have a non-null Response.getEntity(). IMHO this is the minimum we can support right now. I'll post a rev link shortly

      Cheers, Sergey

      Show
      Sergey Beryozkin added a comment - Updating ResponseBuilder is not realistic. I've actually been poking around it too right now, committing shortly. The current solution I came up with is to validate a single bean only if we have a non-null Response.getEntity(). IMHO this is the minimum we can support right now. I'll post a rev link shortly Cheers, Sergey
      Hide
      Sergey Beryozkin added a comment -

      Andriy, have a look please:

      http://svn.apache.org/r1539064

      IMHO it is reasonable to expect that if we have two methods one returning BookWithValidation and another Response with BookWithValidation and some extra header then we have BookWithValidation validated in both cases

      Show
      Sergey Beryozkin added a comment - Andriy, have a look please: http://svn.apache.org/r1539064 IMHO it is reasonable to expect that if we have two methods one returning BookWithValidation and another Response with BookWithValidation and some extra header then we have BookWithValidation validated in both cases
      Hide
      Sergey Beryozkin added a comment -

      I also added a general utility method to Validator so that the custom code can validate arbitrary beans whenever it is needed

      Show
      Sergey Beryozkin added a comment - I also added a general utility method to Validator so that the custom code can validate arbitrary beans whenever it is needed
      Hide
      Andriy Redko added a comment -

      Excellent, I will take a look tonight. You are very right about ResponseBuilder, I thought it's part of CXF implementation but it's part of API, change is not possible for sure.
      Thanks.

      Show
      Andriy Redko added a comment - Excellent, I will take a look tonight. You are very right about ResponseBuilder, I thought it's part of CXF implementation but it's part of API, change is not possible for sure. Thanks.
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      I have reviewed the code and would like to express one concern with respect to following the specification (Chapter 7): it confirms the expectation about the moment when validation takes place. It says "The presence of @Valid will trigger validation of all the constraint annotations decorating a Java bean class". But when we do Response validation, we don't check for @Valid presence (which also means validation cannot be turned off). I like your idea but would suggest to have this additional check: if method's return value is Response(), entity != null and method's return value is annotated with @Valid. What do you think?

      Thank you.

      Show
      Andriy Redko added a comment - Hi Sergey, I have reviewed the code and would like to express one concern with respect to following the specification (Chapter 7): it confirms the expectation about the moment when validation takes place. It says "The presence of @Valid will trigger validation of all the constraint annotations decorating a Java bean class". But when we do Response validation, we don't check for @Valid presence (which also means validation cannot be turned off). I like your idea but would suggest to have this additional check: if method's return value is Response(), entity != null and method's return value is annotated with @Valid. What do you think? Thank you.
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      While looking for generic Response validation, I found out that our exception mapper is not complete. It covers only ConstraintViolationException but not ValidationException (quote from spec: If the exception is of type javax.validation.ValidationException or any of its subclasses excluding
      javax.validation.ConstraintViolationException, then it is mapped to a response
      with status code 500 (Internal Server Error)). I have fixed that by renaming ConstraintViloationExceptionMapper to ValidationExceptionMapper and handling this additional requirement as well. Please review this patch (patch-validation-exception-mapper.txt).

      Thanks a lot.

      Show
      Andriy Redko added a comment - Hi Sergey, While looking for generic Response validation, I found out that our exception mapper is not complete. It covers only ConstraintViolationException but not ValidationException (quote from spec: If the exception is of type javax.validation.ValidationException or any of its subclasses excluding javax.validation.ConstraintViolationException, then it is mapped to a response with status code 500 (Internal Server Error)). I have fixed that by renaming ConstraintViloationExceptionMapper to ValidationExceptionMapper and handling this additional requirement as well. Please review this patch (patch-validation-exception-mapper.txt). Thanks a lot.
      Hide
      Sergey Beryozkin added a comment -

      Hi, well spotted, patch has been applied, thanks.

      Show
      Sergey Beryozkin added a comment - Hi, well spotted, patch has been applied, thanks.
      Hide
      Sergey Beryozkin added a comment - - edited

      Updated JAX-RS interceptors to support JAX-RS 2.0 filter contracts:
      http://svn.apache.org/r1539330

      This can let users apply them selectively to individual methods from 2.0 DynamicFeature. In some cases it will likely be easier to register them as filters anyway

      Show
      Sergey Beryozkin added a comment - - edited Updated JAX-RS interceptors to support JAX-RS 2.0 filter contracts: http://svn.apache.org/r1539330 This can let users apply them selectively to individual methods from 2.0 DynamicFeature. In some cases it will likely be easier to register them as filters anyway
      Hide
      Sergey Beryozkin added a comment -

      Andriy, can we assume "violation.getPropertyPath()" indirectly indicates the position of the parameter by using a convention like "arg1", etc ?

      thanks, Sergey

      Show
      Sergey Beryozkin added a comment - Andriy, can we assume "violation.getPropertyPath()" indirectly indicates the position of the parameter by using a convention like "arg1", etc ? thanks, Sergey
      Hide
      Andriy Redko added a comment -

      Yes, we can assume that. Similar approach is also valid for collections (f.e. if 2-nd element of collection is not valid).
      Thanks.

      Show
      Andriy Redko added a comment - Yes, we can assume that. Similar approach is also valid for collections (f.e. if 2-nd element of collection is not valid). Thanks.
      Hide
      Andriy Redko added a comment -
      Show
      Andriy Redko added a comment - Sorry if you already covered it, but would you address / disagree with this one: https://issues.apache.org/jira/browse/CXF-5309?focusedCommentId=13814495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13814495 Thanks. Andriy.
      Hide
      Sergey Beryozkin added a comment -

      Sorry, missed it, yes you are right, I fixed it. Thanks .

      Re getting the actual arg names, I wonder if we should re-introduce your ServerConstraintViolationException (I'd only rename it to RequestConstraintViolationException) so that it can capture an invoked method and may be keep a list of correct parameter names, so that default CXF and custom mappers would check if it is an instance of RequestConstraintViolationException and if yes then get the name of the parameter given a violation.getPropertyPath() as a key ?

      Cheers, Sergey

      Show
      Sergey Beryozkin added a comment - Sorry, missed it, yes you are right, I fixed it. Thanks . Re getting the actual arg names, I wonder if we should re-introduce your ServerConstraintViolationException (I'd only rename it to RequestConstraintViolationException) so that it can capture an invoked method and may be keep a list of correct parameter names, so that default CXF and custom mappers would check if it is an instance of RequestConstraintViolationException and if yes then get the name of the parameter given a violation.getPropertyPath() as a key ? Cheers, Sergey
      Hide
      Andriy Redko added a comment -

      Hey Sergey,

      I will try to find the generic solution to cover this use case (I guess even standalone applications do have same problem).
      If it is not possible, I will try to follow your suggestion.

      Thanks.
      Andriy

      Show
      Andriy Redko added a comment - Hey Sergey, I will try to find the generic solution to cover this use case (I guess even standalone applications do have same problem). If it is not possible, I will try to follow your suggestion. Thanks. Andriy
      Hide
      Sergey Beryozkin added a comment -

      Sounds good.
      FYI, I updated JAXRSValidationInvoker only for now to optionally validate service objects too (Phase 2 in Chapter 7). The interceptors would typically deal with singletons, and as such I'm not sure yet if we should validate them at the interceptor level or better expect Spring or CDI do it. If needed we can easily add a concurrent map holding singleton validation statuses to AbstractValidationInInterceptor, but for now lets not do it

      Cheers, Sergey

      Show
      Sergey Beryozkin added a comment - Sounds good. FYI, I updated JAXRSValidationInvoker only for now to optionally validate service objects too (Phase 2 in Chapter 7). The interceptors would typically deal with singletons, and as such I'm not sure yet if we should validate them at the interceptor level or better expect Spring or CDI do it. If needed we can easily add a concurrent map holding singleton validation statuses to AbstractValidationInInterceptor, but for now lets not do it Cheers, Sergey
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      I have looked on a ways to preserve method argument names. Obviously, they are not kept unless debug build is being used.
      Hibernate Validator will include parameter names provider based on Paranamer project (https://github.com/paul-hammant/paranamer), respective implementation: https://github.com/hibernate/hibernate-validator/blob/master/engine/src/main/java/org/hibernate/validator/parameternameprovider/ParanamerParameterNameProvider.java. Basically, technique is well-known and based on annotation processors. If you agree, I suggest not to spend time on that now (and reuse the later implementation when it's released) unless we see it as a critical issue. What do you think?

      Thanks.
      Andriy

      Show
      Andriy Redko added a comment - Hi Sergey, I have looked on a ways to preserve method argument names. Obviously, they are not kept unless debug build is being used. Hibernate Validator will include parameter names provider based on Paranamer project ( https://github.com/paul-hammant/paranamer ), respective implementation: https://github.com/hibernate/hibernate-validator/blob/master/engine/src/main/java/org/hibernate/validator/parameternameprovider/ParanamerParameterNameProvider.java . Basically, technique is well-known and based on annotation processors. If you agree, I suggest not to spend time on that now (and reuse the later implementation when it's released) unless we see it as a critical issue. What do you think? Thanks. Andriy
      Hide
      Sergey Beryozkin added a comment - - edited

      Hi Andriy, I agree it is not a high priority issue. I'll play a bit later with it and see how realistic it is to get some useful info from the existing OperationResourceInfo based on the assumption that the violation path will correctly identify the parameter position.

      I think we are nearly done then as far as this JIRA is concerned, for now at least, right ?

      What I'd like to do as well is try Apache BVal 1.1 branch against the test which you added, it is important for us to be able to say in our docs what Bean Validation 1.1 implementations can be used; let me know please if it would of interest to you, or may be you can think of what else can be tested or implemented ?

      Thanks, Sergey

      Show
      Sergey Beryozkin added a comment - - edited Hi Andriy, I agree it is not a high priority issue. I'll play a bit later with it and see how realistic it is to get some useful info from the existing OperationResourceInfo based on the assumption that the violation path will correctly identify the parameter position. I think we are nearly done then as far as this JIRA is concerned, for now at least, right ? What I'd like to do as well is try Apache BVal 1.1 branch against the test which you added, it is important for us to be able to say in our docs what Bean Validation 1.1 implementations can be used; let me know please if it would of interest to you, or may be you can think of what else can be tested or implemented ? Thanks, Sergey
      Hide
      Andriy Redko added a comment -

      Hey Sergey,

      Thanks a lot. Sure, I am definitely interested to validate our implementation against Apache BVal 1.1 and will take that.
      I am also would like to validate integration with Spring and add test cases for that.

      Thanks.
      Andriy

      Show
      Andriy Redko added a comment - Hey Sergey, Thanks a lot. Sure, I am definitely interested to validate our implementation against Apache BVal 1.1 and will take that. I am also would like to validate integration with Spring and add test cases for that. Thanks. Andriy
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      I've switched the CXF to Apache BVal (https://svn.apache.org/repos/asf/bval/branches/bval-11/). From compilation / discovery standpoint - everything works seamlessly. Unfortunately, BVal 1.1 is not ready yet, I have got many weird runtime exceptions while bean annotations inspection happens (some weird casting of Annotation class to String), but I was able to run application / systests (though all failed because of mentioned issues). I guess it needs some time but I don't see any single reason why it shouldn't eventually work out (we don't introduce any dependency to Hibernate-specific classes, strongly on 1.1 API).

      Working on Spring-like test case right now, would like to verify our dependencies injection across service beans / interceptors.
      Thanks.

      Andriy.

      Show
      Andriy Redko added a comment - Hi Sergey, I've switched the CXF to Apache BVal ( https://svn.apache.org/repos/asf/bval/branches/bval-11/ ). From compilation / discovery standpoint - everything works seamlessly. Unfortunately, BVal 1.1 is not ready yet, I have got many weird runtime exceptions while bean annotations inspection happens (some weird casting of Annotation class to String), but I was able to run application / systests (though all failed because of mentioned issues). I guess it needs some time but I don't see any single reason why it shouldn't eventually work out (we don't introduce any dependency to Hibernate-specific classes, strongly on 1.1 API). Working on Spring-like test case right now, would like to verify our dependencies injection across service beans / interceptors. Thanks. Andriy.
      Hide
      Sergey Beryozkin added a comment -

      Hi Andriy,

      Thanks for checking Apache BVal 1.1, yes, we can wait till it gets completed and then retry.

      Cheers, Sergey

      Show
      Sergey Beryozkin added a comment - Hi Andriy, Thanks for checking Apache BVal 1.1, yes, we can wait till it gets completed and then retry. Cheers, Sergey
      Hide
      Sergey Beryozkin added a comment -

      Hi Andriy, All

      I've spent quite a bit of time trying to get some more useful information into the logs, specifically on how to minimize the amount of the extra information so that the actual violation message does not get lost in the noise. Here is what I've come up so far, the extra info is optionally added in '()', examples:

      1. Non request body parameter validation issue (FormParam, PathParam, etc):

      SEVERE: BookStoreWithValidation.addBook.arg1(JAXRS param is FORM("id"), class: String): may not be null
      

      3. Request Body (example, XML converted to JAXB bean) validation issue

      SEVERE: BookStoreWithValidation.addBookDirect.arg0.name(arg0 JAXRS param is REQUEST_BODY, class: BookWithValidation): may not be null
      

      3. Collection Request Body validation issue

      SEVERE: BookStoreWithValidation.addBooksDirect.arg0[0].name(arg0 JAXRS param is REQUEST_BODY, class: List): may not be null
      
      
      Show
      Sergey Beryozkin added a comment - Hi Andriy, All I've spent quite a bit of time trying to get some more useful information into the logs, specifically on how to minimize the amount of the extra information so that the actual violation message does not get lost in the noise. Here is what I've come up so far, the extra info is optionally added in '()', examples: 1. Non request body parameter validation issue (FormParam, PathParam, etc): SEVERE: BookStoreWithValidation.addBook.arg1(JAXRS param is FORM("id"), class: String): may not be null 3. Request Body (example, XML converted to JAXB bean) validation issue SEVERE: BookStoreWithValidation.addBookDirect.arg0.name(arg0 JAXRS param is REQUEST_BODY, class: BookWithValidation): may not be null 3. Collection Request Body validation issue SEVERE: BookStoreWithValidation.addBooksDirect.arg0[0].name(arg0 JAXRS param is REQUEST_BODY, class: List): may not be null
      Hide
      Sergey Beryozkin added a comment -

      Andriy, please review:

      http://svn.apache.org/r1540118

      Note this is disabled by default so far, people will be able to turn it on optionally

      Show
      Sergey Beryozkin added a comment - Andriy, please review: http://svn.apache.org/r1540118 Note this is disabled by default so far, people will be able to turn it on optionally
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      I like your idea. Though your implementation relies heavily on the way Hibernate Validator reports violations, we could do it just using 1.1 API and implementing javax.validation.ParameterNameProvider, call it let's say JAXRSParameterNameProvider and it's guaranteed to work for any implementation. Do you think it worth it? I would be able to do that.

      Thanks.
      Andriy

      Show
      Andriy Redko added a comment - Hi Sergey, I like your idea. Though your implementation relies heavily on the way Hibernate Validator reports violations, we could do it just using 1.1 API and implementing javax.validation.ParameterNameProvider, call it let's say JAXRSParameterNameProvider and it's guaranteed to work for any implementation. Do you think it worth it? I would be able to do that. Thanks. Andriy
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      I've created a small test cases to validate Spring integration and manual validation work fine (patch-validation-spring-tests.txt). Could you please review it and apply if it's OK?

      Thanks a lot.
      Andriy

      Show
      Andriy Redko added a comment - Hi Sergey, I've created a small test cases to validate Spring integration and manual validation work fine (patch-validation-spring-tests.txt). Could you please review it and apply if it's OK? Thanks a lot. Andriy
      Hide
      Sergey Beryozkin added a comment -

      Hi Andriy,

      Re the extra parameter info. You are right, we don;t know yet if it will work with Apache BVal for example, though what is on the plus side is that it is an optional feature disabled by default, so if people do not want it or work with Hibernate then they won;t even worry about enabling it . We can work with the ApacheBVal team to make sure the default resourcePath Apache BVal produces uses the same conventions, I'd actually expect BVal producing the same conventions, but I'm not sure yet.

      I did not know ParameterHandlerProvider was available in the API, thanks for the hint. I'm not sure it should return more than the actual Java arg names though, but I guess we should indeed update ValidationProvider to use it to create the validators if it was injected via the ValidationProvider setter ? Please create a patch

      Thanks, Sergey

      Show
      Sergey Beryozkin added a comment - Hi Andriy, Re the extra parameter info. You are right, we don;t know yet if it will work with Apache BVal for example, though what is on the plus side is that it is an optional feature disabled by default, so if people do not want it or work with Hibernate then they won;t even worry about enabling it . We can work with the ApacheBVal team to make sure the default resourcePath Apache BVal produces uses the same conventions, I'd actually expect BVal producing the same conventions, but I'm not sure yet. I did not know ParameterHandlerProvider was available in the API, thanks for the hint. I'm not sure it should return more than the actual Java arg names though, but I guess we should indeed update ValidationProvider to use it to create the validators if it was injected via the ValidationProvider setter ? Please create a patch Thanks, Sergey
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      Yes, the step of building validation provider instance includes the ability to specify the ParameterNameProvider so we can inject it inside ValidationProvider setter or constructor. Will try to come up with the patch ASAP.

      Thanks.
      Andriy

      Show
      Andriy Redko added a comment - Hi Sergey, Yes, the step of building validation provider instance includes the ability to specify the ParameterNameProvider so we can inject it inside ValidationProvider setter or constructor. Will try to come up with the patch ASAP. Thanks. Andriy
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      Please review the patch for JAXRSParameterNameProvider (patch-validation-paramnameprovider.txt). I've followed your implementation in a way to decorate parameters with JAX-RS specific metadata. Also, there are test cases with this parameter name provider injected (I've used the construction injection to keep it simple).

      Sample output:
      SEVERE: BookStoreWithValidation.addBook.arg1(JAXRS param is FORM("id"), class: String): may not be null

      Thanks.
      Andriy

      Show
      Andriy Redko added a comment - Hi Sergey, Please review the patch for JAXRSParameterNameProvider (patch-validation-paramnameprovider.txt). I've followed your implementation in a way to decorate parameters with JAX-RS specific metadata. Also, there are test cases with this parameter name provider injected (I've used the construction injection to keep it simple). Sample output: SEVERE: BookStoreWithValidation.addBook.arg1(JAXRS param is FORM("id"), class: String): may not be null Thanks. Andriy
      Hide
      Sergey Beryozkin added a comment - - edited

      Hi Andriy, thanks for the last 2 patches, I'll work on them shorty.

      Regarding the parameter name handler injection, should we also support a setter ? The constructor injection is fine (lets keep this single new constructor), but we'd need to have more constructors added to cover the cases where we have multiple providers available, given that the provider has additional constructors for supporting non-default factory initializations and doing the constructor injection with more than 1 arg when it might be needed is not easy from Spring .

      What about additionally supporting a setter and then, for example,

      private< T > Set<ConstraintViolation< T > > doValidateBean(final T bean) {
              return getValidator().validate(bean);
       }
          
      private ExecutableValidator getExecutableValidator() {
              
             return getValidator().forExecutables();
      }
      
      private Validator getValidator() {
             if (paramHandler != null) {
                 return factory.usingContext().parameterNameProvider(paramHandler).getValidator();
             } else {
                 return factory.getValidator();
             }
      }
      

      Will it work for you ?
      Thanks, Sergey

      Show
      Sergey Beryozkin added a comment - - edited Hi Andriy, thanks for the last 2 patches, I'll work on them shorty. Regarding the parameter name handler injection, should we also support a setter ? The constructor injection is fine (lets keep this single new constructor), but we'd need to have more constructors added to cover the cases where we have multiple providers available, given that the provider has additional constructors for supporting non-default factory initializations and doing the constructor injection with more than 1 arg when it might be needed is not easy from Spring . What about additionally supporting a setter and then, for example, private < T > Set<ConstraintViolation< T > > doValidateBean( final T bean) { return getValidator().validate(bean); } private ExecutableValidator getExecutableValidator() { return getValidator().forExecutables(); } private Validator getValidator() { if (paramHandler != null ) { return factory.usingContext().parameterNameProvider(paramHandler).getValidator(); } else { return factory.getValidator(); } } Will it work for you ? Thanks, Sergey
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      Yep, it definitely works for me. Thanks a lot.
      Please apply this patch only (patch-validation-paramnameprovider.txt), another one is included in it as well.

      Thanks!
      Andriy

      Show
      Andriy Redko added a comment - Hi Sergey, Yep, it definitely works for me. Thanks a lot. Please apply this patch only (patch-validation-paramnameprovider.txt), another one is included in it as well. Thanks! Andriy
      Hide
      Sergey Beryozkin added a comment -

      Hi Andrei,

      I applied part of your patch related to the name provider, I'll deal with the tests a bit later, I just want to add a JAXRSInvoker test there as well in order to test that the actual (per-request) service bean having the JAX-RS parameters injected into it can also be validated.

      Re the name providers, I stopped short of introducing setters, they seemed a bit redundant to me, it does not appear any more difficult to use a 'constructor-arg' instead of 'property', I exaggerated a bit there in my earlier comments.

      I also added ValidationConfiguration so that users can configure the factory as much as they like.

      Also removed the custom parameter info code from ValidationExceptionMapper, but also relaxed a bit the LOG level to WARNING, I guess we should use SEVERE only when the ValidatorProvider can not do its work

      Thanks, Sergey

      Show
      Sergey Beryozkin added a comment - Hi Andrei, I applied part of your patch related to the name provider, I'll deal with the tests a bit later, I just want to add a JAXRSInvoker test there as well in order to test that the actual (per-request) service bean having the JAX-RS parameters injected into it can also be validated. Re the name providers, I stopped short of introducing setters, they seemed a bit redundant to me, it does not appear any more difficult to use a 'constructor-arg' instead of 'property', I exaggerated a bit there in my earlier comments. I also added ValidationConfiguration so that users can configure the factory as much as they like. Also removed the custom parameter info code from ValidationExceptionMapper, but also relaxed a bit the LOG level to WARNING, I guess we should use SEVERE only when the ValidatorProvider can not do its work Thanks, Sergey
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      Awesome, thanks! Once everything is merged + new tests cases added, we could think of PoC to be finished, what is your opinion?
      Thanks.

      Andriy.

      Show
      Andriy Redko added a comment - Hi Sergey, Awesome, thanks! Once everything is merged + new tests cases added, we could think of PoC to be finished, what is your opinion? Thanks. Andriy.
      Hide
      Sergey Beryozkin added a comment -

      I agree, POC was completed after I applied your 1st patch , but when we close this issue (with its master JIRA) we will say that Bean Validation is completely supported for CXF and the JAX-RS frontened specifically.
      Thanks, Sergey

      Show
      Sergey Beryozkin added a comment - I agree, POC was completed after I applied your 1st patch , but when we close this issue (with its master JIRA) we will say that Bean Validation is completely supported for CXF and the JAX-RS frontened specifically. Thanks, Sergey
      Hide
      Andriy Redko added a comment -

      Totally on that, I meant - no show-stoppers / blockers to have this feature fully implemented. Would you like me to help you further with something?
      Thanks.
      Andriy

      Show
      Andriy Redko added a comment - Totally on that, I meant - no show-stoppers / blockers to have this feature fully implemented. Would you like me to help you further with something? Thanks. Andriy
      Hide
      Sergey Beryozkin added a comment -

      If you could take on a JAXRSInvoker test then it would be nice .

      @Path("/bookstore/")
      public class BookStoreWithValidationPerRequest  {
          private Map<String, BookWitthValidation> books = 
              Collections.singletonMap("123", new BookWitthValidation("CXF", 123L)); 
          
          private String id;
          
          public BookStoreWithValidationPerRequest() {
          }
      
          @QueryParam("id")
          public void setId(String id) {
              this.id = id;
          }
      
          @Valid
          public String getId() {
              return this.id;
          }
          
          @GET
          @Path("books")
          public BookWithValidation getBook() {
              return books.get(id);
          }
      }
      

      and JAXRSValidationInvoker will enforce that before BookStoreWithValidationPerRequest#getBook is invoked, the exception will be thrown if BookStoreWithValidationPerRequest#id does not match some validation rules.

      You can create say JAXRSPerRequestValidationTest, copy the server setup from JAXRSClientServerValidationTest, refer to BookStoreWithValidationPerRequest and drop a setResourceProvider call (so the default per-request lifecycle mode will be activated) and then I guess add just a single test for a start.

      Give it a go please
      Thanks, Sergey

      Show
      Sergey Beryozkin added a comment - If you could take on a JAXRSInvoker test then it would be nice . @Path( "/bookstore/" ) public class BookStoreWithValidationPerRequest { private Map< String , BookWitthValidation> books = Collections.singletonMap( "123" , new BookWitthValidation( "CXF" , 123L)); private String id; public BookStoreWithValidationPerRequest() { } @QueryParam( "id" ) public void setId( String id) { this .id = id; } @Valid public String getId() { return this .id; } @GET @Path( "books" ) public BookWithValidation getBook() { return books.get(id); } } and JAXRSValidationInvoker will enforce that before BookStoreWithValidationPerRequest#getBook is invoked, the exception will be thrown if BookStoreWithValidationPerRequest#id does not match some validation rules. You can create say JAXRSPerRequestValidationTest, copy the server setup from JAXRSClientServerValidationTest, refer to BookStoreWithValidationPerRequest and drop a setResourceProvider call (so the default per-request lifecycle mode will be activated) and then I guess add just a single test for a start. Give it a go please Thanks, Sergey
      Hide
      Andriy Redko added a comment -

      Sure, I will take it. Thanks.
      Andriy.

      Show
      Andriy Redko added a comment - Sure, I will take it. Thanks. Andriy.
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      Please review the patch (patch-jaxrsvalidationinvoker-tests.txt) with initial set of JAXRSValidationInvoker tests. I have fixed a small issue with return value validation, thanks to those test cases.

      Thanks.
      Andriy.

      Show
      Andriy Redko added a comment - Hi Sergey, Please review the patch (patch-jaxrsvalidationinvoker-tests.txt) with initial set of JAXRSValidationInvoker tests. I have fixed a small issue with return value validation, thanks to those test cases. Thanks. Andriy.
      Hide
      Sergey Beryozkin added a comment -

      Hi Andriy, many thanks, I've applied your patch with few minor updates (example, JAXRSInvoker would always return MessageContentsList, plus few other minor updates).

      What I've realized, though I guess we did touch on it earlier, is that if we have JAX-RS Response wrapping an entity then the only thing we can do at the moment is to validate a non-null entity itself, assuming it has the embedded constraints, but we always miss on method-level annotations such as @NotNull.

      When we have a null Response entity we can check if method annotations have @NotNull and if yes then return a custom NotNulConstraintViolation. Thus we can cover the NotNull validation at least. But I wonder if we can do better than that ? Have a look please at ValidatorProvider#validateResponse(Method m, Object returnV), can we iterate over all the method annotations, check for the ones with Constraints, if yes, then invoke the attached ConstraintValidator. Can that work at all ? It is not a critical issue, if it is not possible to do then I can add a NotNull check only and then resolve this issue

      Thanks, Sergey

      Show
      Sergey Beryozkin added a comment - Hi Andriy, many thanks, I've applied your patch with few minor updates (example, JAXRSInvoker would always return MessageContentsList, plus few other minor updates). What I've realized, though I guess we did touch on it earlier, is that if we have JAX-RS Response wrapping an entity then the only thing we can do at the moment is to validate a non-null entity itself, assuming it has the embedded constraints, but we always miss on method-level annotations such as @NotNull. When we have a null Response entity we can check if method annotations have @NotNull and if yes then return a custom NotNulConstraintViolation. Thus we can cover the NotNull validation at least. But I wonder if we can do better than that ? Have a look please at ValidatorProvider#validateResponse(Method m, Object returnV), can we iterate over all the method annotations, check for the ones with Constraints, if yes, then invoke the attached ConstraintValidator. Can that work at all ? It is not a critical issue, if it is not possible to do then I can add a NotNull check only and then resolve this issue Thanks, Sergey
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      Thanks a lot. Yeah, such a cases were my concerns when we have discussed Response validation some time ago and added @Valid annotation presence check.
      I will try to spend more time on it over week-end and suggest the solution suitable for most cases (or at worst prove we have a strong reasons not to implement it).

      Thanks.
      Andriy

      Show
      Andriy Redko added a comment - Hi Sergey, Thanks a lot. Yeah, such a cases were my concerns when we have discussed Response validation some time ago and added @Valid annotation presence check. I will try to spend more time on it over week-end and suggest the solution suitable for most cases (or at worst prove we have a strong reasons not to implement it). Thanks. Andriy
      Hide
      Sergey Beryozkin added a comment - - edited

      Thanks Andriy, please take your time. It appears it is not possible to obtain ConstrainValidatorContext, but it appears 'null' can be passed just fine to Constraint annotations, so it might work, lets see
      Cheers. Sergey

      Show
      Sergey Beryozkin added a comment - - edited Thanks Andriy, please take your time. It appears it is not possible to obtain ConstrainValidatorContext, but it appears 'null' can be passed just fine to Constraint annotations, so it might work, lets see Cheers. Sergey
      Hide
      Sergey Beryozkin added a comment -

      Hi Andriy, I'm resolving this issue as Fixed for the upcoming 3.0 milestone release as we have mostly implemented it.
      Please check
      https://issues.apache.org/jira/browse/CXF-5401

      We can target Response related validation with CXF-5401.

      Major thanks for all your help with getting Bean Validation 1.1 wired in

      Show
      Sergey Beryozkin added a comment - Hi Andriy, I'm resolving this issue as Fixed for the upcoming 3.0 milestone release as we have mostly implemented it. Please check https://issues.apache.org/jira/browse/CXF-5401 We can target Response related validation with CXF-5401 . Major thanks for all your help with getting Bean Validation 1.1 wired in
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      Got it. Sorry, I have quite a busy week-end and was not able to come up with a good solution for it.
      But I do have at least 2 ideas how we could tackle this problem, not sure these will work though, haven't tried it yet (I looked through JSR-349 to understand the options we have, it took some time).
      Do you want me to continue and post my comments under 5401?

      Thanks a lot.
      Andriy

      Show
      Andriy Redko added a comment - Hi Sergey, Got it. Sorry, I have quite a busy week-end and was not able to come up with a good solution for it. But I do have at least 2 ideas how we could tackle this problem, not sure these will work though, haven't tried it yet (I looked through JSR-349 to understand the options we have, it took some time). Do you want me to continue and post my comments under 5401? Thanks a lot. Andriy
      Hide
      Sergey Beryozkin added a comment -

      No problems at all, I've resolved it for Dan to update the release notes, the Response issue can definitely be resolved in the next release, yes please comment under 5401 when you get a chance

      Thanks, Sergey

      Show
      Sergey Beryozkin added a comment - No problems at all, I've resolved it for Dan to update the release notes, the Response issue can definitely be resolved in the next release, yes please comment under 5401 when you get a chance Thanks, Sergey
      Hide
      Sergey Beryozkin added a comment - - edited

      Hi Andriy, All,

      I was wondering if we should rename all validation interceptors and add 'Bean' into the names. Example, we have ValidationInInterceptor, ValidationOutInterceptor, ValidationFeature, etc. CXF also supports the schema validation. So 'BeanValidationInInterceptor', etc would read better/clearer. Any objections if I rename the current interceptors, before we start documenting it ?
      Sergey

      Show
      Sergey Beryozkin added a comment - - edited Hi Andriy, All, I was wondering if we should rename all validation interceptors and add 'Bean' into the names. Example, we have ValidationInInterceptor, ValidationOutInterceptor, ValidationFeature, etc. CXF also supports the schema validation. So 'BeanValidationInInterceptor', etc would read better/clearer. Any objections if I rename the current interceptors, before we start documenting it ? Sergey
      Hide
      Andriy Redko added a comment -

      Hi Sergey,

      In my opinion, it sounds like a good idea, it helps to distinguish this kind of validation from, f.e. schema / wsdl / etc validations.
      Thanks.

      Andriy

      Show
      Andriy Redko added a comment - Hi Sergey, In my opinion, it sounds like a good idea, it helps to distinguish this kind of validation from, f.e. schema / wsdl / etc validations. Thanks. Andriy
      Hide
      Sergey Beryozkin added a comment -

      Done.

      Show
      Sergey Beryozkin added a comment - Done.

        People

        • Assignee:
          Sergey Beryozkin
          Reporter:
          Sergey Beryozkin
        • Votes:
          1 Vote for this issue
          Watchers:
          4 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development