Re: CXF BeanValidation annotation support

[Sergey Beryozkin]

Hi Johannes

Thanks, while I accept I may be wrong I'd rather us indeed have a bit of 
break and reflect on all the 3 available options - perhaps some ideas 
will come later on

Enjoy the weekends
Sergey
On 18/11/16 22:32, J. Fiala wrote:
Hi Sergey,

Thank you for explaining all of this once more, I'll rethink this and
then come back to you :).

Thanks & Best regards,
Johannes


Am 18.11.2016 um 22:17 schrieb Sergey Beryozkin:
Hi Johannes

As I said we've discussed all the options in depth twice and as far as
I recall we eve came to some conclusion with respect to an approach
where incomplete XMLSchema fragments are used to model the Java bean
validation - where I asked you to seek some support from the original
Sun project.

But let me explain my position one more time.

Option 1. Use incomplete XML Schema fragments - IMHO this is a
non-interoperable and at least IMHO is a stretch with some obvious
limitations such as the one related to your idea of using some
embedded XML schema types to model something more complex than NotNull
or the fact that schema fragments are incomplete, and it is hard to
make it work both ways.

Option2. Introduce a CXF extension designed for representing whatever
info needed related to the validation of bean parameters - this will
scale and IMHO will have a better chance to eventually become a more
accepted generic solution.

You prefer Option1, I Option2, we keep going in circles :-), and at
this stage IMHO it will be good for us to politely agree to disagree.

The other thing I'd like note though is as I said earlier, the whole
idea of making the optional parameters (query params are optional by
theor nature) strongly required is questionable IMHO.

If you have a service at
/bar

then this service, being a good HTTP service, should be able to accept
/bar without /bar?a=c.

This is what for ex JAX-RS DefaultValue is for. Or a good service
would simply return the complete /bar representation if an optional
parameter is missing.

Enforcing the presence of 'a' effectively makes your top level API
being not "/bar" but "/bar?a=..." which is not good IMHO.

Providing some top level support in CXF does seem to me a good
investment of time.

Finally, you have option 3:
@GET
someMethod(@QueryParam("a") MyBean bean) {}

This will work OOB for wadl-to-java and will be trivial enough to
support for Java to Wadl. Works both ways, standard JAX-RS 2.0, you
control the instantiation of MyBean in your custom
ParamConverter<MyBean> and there you will validate this bean as needed.

Hope you appreciate I've tried to justify my position as much as I can.
Sometimes it is good to have a break in discussions which go too long,
think about the whole issue a bit more and then may be revisit it later

Thanks, Sergey


On 18/11/16 21:27, J. Fiala wrote:
Hi Sergey,

So far you can reference any simpletype at the WADL, but it seems the
idea of adding picking up the restrictions and using it for
beanvalidation is new (I found one related question dated 2006:
http://markmail.org/message/2pih4jwd2dn6xidv#query:+page:1+mid:6wjzispjnd2bfnyq+state:results).



However, I think this would be the most intuitive way and would also
allow users to reuse existing simpletypes in their schema they use in
complextypes (I know I already said this...). From this perspective
adding a separate cxf namespace would be counter-intuitive and would
disallow users to pick up their existing simpletypes (e.g. if they
defined a simpletype nameType with maximum of 255 chars and they want to
use this both for a search parameter and in a complextype personType).

Also from the Swagger-perspective Swagger has a separate parameterType
where you can define their restrictions, so they also reuse the existing
mechanisms of the Swagger contract and don't add new stuff for this.

Even if you prefer setting up a cxf-namespace, users with existing
reference to simpletypes in their parameters would then complain that
they have to add separate cxf:xx tags to get the beanvalidation support.
So I think you need to support both anyway.

The only funny thing is that no one has thrown this up in the last years
here (except the question I found at the link above, but unfortunately
the user needed a binary decision not a range) - in the Swagger-spec
this is just a natural thing - adding restrictions to parameters, and I
think it should be exactly like this!

But I'll do more research on this and hopefully come back with more hard
facts on interoperability .... :).

I'll put a hold on this until we have come to a decision, I just needed
the new PR to move to a separate branch to get a clean master to get the
lastest updates :).

best regards,
Johannes


Am 08.11.2016 um 18:11 schrieb J. Fiala:
Hi Sergey,

Thx for all the feedback. I think this is a good idea - check what
other projects are doing / generating WADLs to see how we can make
this cross-framework in a way that makes sense for portability.

Best regards,
Johannes


Am 08.11.2016 um 12:33 schrieb Sergey Beryozkin:
And finally, I'm not saying inlining XML schema fragments is going
nowhere, if you are passionate about this particular approach then
please try to get some positive feedback form a team supporting an
original Sun wadl-to-java tool - I'll support you at the CXF level if
you will some progress.

But in meantime I'd like to encourage you to review Option 1 & 2 I
listed earlier deeper. IMHO Option1 has a higher chance of getting an
eventual wider adoption (with a diff namespace of course)

Cheers, Sergey
On 08/11/16 11:22, Sergey Beryozkin wrote:
Hi Johannes

I'd like to clarify.

Rather than going the XML schema path, with the incomplete but
possibly
nested schema types to keep even this already invalid incomplete
schema
fragment being a single 'piece', and effectively starting modeling
BeanValidation via XML schema, and trying to figure out how to
support
it cleanly both ways, all for the sake of letting the user to avoid
doing a 'null' check or some size check in the code,

I'd rather like to see a simple solution which works and which
'speaks'
it is about a parameter validation.

The fact that the Swagger2 supports this approach is not convincing.
And
now that I think about it it is even unusual to hear about Swagger
inlining somewhere incomplete XML schema fragments, being itself very
much JSON centric.

So I'd not even call this proposed extension a 'beanValidation' but
simply 'validation':
<wadl ... xmlns:val="http://cxf.apache.org/wadl/validation";>
<wadl:param>
 <val:validation>
    ...
 </val:validation>
</wadl:param>

I look at it and see it is about the extra validation of a given
non-representation (non XMLSchema related) parameter.

If XMLSchema is a must for you then simply follow an Option2 I
outlined
earlier. It already works on a WADL to Java a path. The only
compromise
you'd have to make is to have non-simple parameter types in the
signature and register ParamConverterProvider. This is still a
perfectly
valid JAX-RS 2.0.

Cheers, Sergey


On 08/11/16 10:25, Sergey Beryozkin wrote:
Hi
On 07/11/16 22:52, J. Fiala wrote:
Hi Sergey,

I think we should split the whole thing into two topics
(issues/PRs):

Well. I'd rather have a single approach working both ways.

1.) SourceGenerator for contract-first-only with 100% portability
This should be supported for people who only practice contract
first and
don't care of the WadlGenerator and simply want to use clean
simple/nested types for their WADL parameters. This warrants 100%
portability for the WADL contract.

a) support simpletypes:

<param name="username" style="query" type="myns:username"
required="false" />

<xs:simpleType name="username">
        <xs:restriction base="xs:string">
            <xs:maxLength value="50"/>
        </xs:restriction>
    </xs:simpleType>

This is what I already implemented, but only for the directly
resolved
type (like example here), here I could go deeper as well to further
restrictions (<xs restriction base="mydeepersimpletype"...>).

b) support nested types:

<param name="username" style="query" type="myns:username"
required="false" >
<xs:simpleType>
<xs:restriction base="xs:string">
<xs:maxLength value="50" />
</xs:restriction>
</param>

This is what I already implemented (check if nested simpleType is
present + pick it up).

If you like to I can rework the PR
https://github.com/apache/cxf/pull/146 and add only the
implementation
of SourceGenerator so you can merge conveniently.

As I said I'm not keen on this particular approach because using an
incomplete XML schema fragments to implicitly carry the optional
parameter validation info, where any of this String/int
parameters has
nothing to do with XML schema, look wrong to me. It does not scale.


2.) BeanValidation for the WadlGenerator and for SourceGenerator
for
easier roundtrip development

It is not the easier roundtrip dev but about having a complete
solution

I think the main task should be to pickup the beanvalidation
annotations
for both wadl:param AND xs:element/xs:complexTypes.
If we only add support for parameters and don't support it in the
complextypes it is only  the minor part of the deal.
However, with the apporach to get working what is possible without
big
effort, we can in first place only support wadl:param.

I honestly would not like to go into the whole XML schema for the
purpose of optionally adding some bean validations, we've spent a
lot of
time talking about it.

For wadl:param:
For @NotNull I'd prefer to use wadl:param@required, no need for the
custom namespace here.
Sure start with supporting NotNull.

So I can start with @Size (minLength/maxLength) here and setup a
first
implementation like the following.

WadlGenerator:
<wadl:param name="username" required="true">
<cxfrs:beanValidation>
  <cxfrs:minLength value="1"/>
  <cxfrs:maxLength value="100" />
</cxfrs:beanValidation>
</wadl:param>
That won't scale once you'd want to add one more annotation
support. It
would have to be something like

<cxfrs:beanValidation>
   <cxfrs:size minLength="1" maxLength="100" />
</cxfrs:beanValidation>

without any nested minLength/etc to simplify the processing of
'size'
and to support adding of other beanVal, including NotNull, ex:

<cxfrs:beanValidation>
   <cxfrs:size minLength="1" maxLength="100" />
   <cxfrs:notNull/>
</cxfrs:beanValidation>


It is better to have a single place for everything related to
beanVal.

SourceGenerator: look for nested beanValidation tag and pick
validations
up.

yes
After implementing this, the next step would be to support the
custom
tag for the complexTypes/JAXB-processing...

I'm sorry I disagree. Lets keep XMLSchema out of the picture for
now as
far as the validation of optional parameters is concerned

Cheers, Sergey
If you agree with that, I suggest setting up two separate Jira
issues so
we can move all the further comments there.

What do you think of this idea?

Best regards,

Johannes



Am 07.11.2016 um 22:12 schrieb Sergey Beryozkin:
Perhaps you can start with supporting a not null only ?
If a parameter @required is set (this can be enabled on the
WADL to
Java if BeanVal NotNull is set) then SourceGenerator can add this
annotation back if its -beanValidation is set.
The only restriction is that please use String class names of the
BeanValidation for now until we figure our a neater approach to
avoid
the strong BeanVal dependencies

Cheers, Sergey
On 07/11/16 21:08, Sergey Beryozkin wrote:
Hi Johannes

Option 2 already works in the WADL to Java case. The only
problem with
it, as discussed earlier, is that WADL grammar is meant to
represent
the
data representations, but SourceGenerator does not enforce it
right
now
and referring from the parameters to the schema elements is still
supported.

IMHO it makes sense to have a solution which works both ways
first
without doing a massive effort as we don't really have
time/scope for
it.

Completing Option2 support will only require adding the
parameter bean
classes to the JAXB context.
Option 1 will require a bit more work at the start but once you
have
something like
<wadl:param>
<cxfrs:beanValidation>
  <notNull/>
  <someOtherBeanValName a="b" "c"="d">
</cxfrs:beanValidation>
</wadl:param>

then it will become quite straigtforward on the WADL to Java
side...

Ideally one would even allocate a namespace which would extend
the
core
WADL namespace. Ex,

http://wadl.dev.java.net/extensions/bval

Might be worth investigating if it is allowed or not.

Cheers, Sergey

On 07/11/16 18:01, J. Fiala wrote:
Hi Sergey,

Thx for the analysis.

Regarding the WadlGenerator I'd also prefer option 1 opposing to
option
2 (without the original idea available).

Regarding the SourceGenerator I think it should support clean
parameter
type references in any way. So even if we follow option 1, we
would
need
to support correctly referenced simpletypes from parameters as
well as
the cxfrs:beanValidation extension.

But probably it's easier to discuss this with examples together
:).

Best regards,
Johannes


Am 07.11.2016 um 18:13 schrieb Sergey Beryozkin:
Hi Johannes

I've looked through various messages and pull requests.

One approach was to insert incomplete schema fragments into
wadl:param
with the implicit understanding it was meant to create BeanVal
parameters and I felt quite uncomfortable about pursuing that
particular path.

The last idea we were trying to narrow upon was using a
separate XML
schema grammar which would be used to create Bean validation
if a
bean
validation flag is set. This is a bit more WADL friendly but
again it
does not look to me like a solution which says "this is to
support
the
bean validation" for parameters and I'm finding it difficult
to see
how it will work both ways.

I'd to revisit these 2 proposals

1. have a CXF WADL extension, ex, cxfrs:beanValidation. This
will be
in wadl:param and will be optimized around supporting all
whatever is
needed to generate BeanVal annotations and represent any
BeanVal in
the Java to WADL case.
This works both ways and IMHO simpler and cleaner, instead of
trying
to 'carry' the bean validation info via XML schema fragments,
with
all
the limitations which seems ultimately wrong to me.
cxfrs:beanValidation will be of course only recognized by a CXF
wadl-to-java tool but I feel this approach will have more
chances to
act getting more support via other provider's tool extensions
than
the
pseudo XML schema approach which is in fact would be equally
CXF
specific.

2. Do not do anything at all but have wadl:param refer to XML
complex
types. As I said earlier, this will require the use of
ParamConverterProvider which will have a chance to validate the
beans.
These beans will already have BeanVal annotations in them.
This also
works both ways - on the WADLToJava side you'd just add these
bean
classes to a JAXBContext for the JAXB schema gen to add the
relevant
schema definitions.

My preference is option 1).

We can discuss the options next week too,

Thanks, Sergey

On 06/11/16 22:25, Sergey Beryozkin wrote:
Hi Johannes

Can you please briefly summarize the proposed changes to
WADLGen &
SourceGen ? There've been many emails and I recall I thought
more
work
was needed to keep both approaches in sync. Right now I see
WADLGen has
some explicit BeanVal dependencies. I def remember
objecting to
this
particular approach.
So how this can be generalized, be WADL friendly, and work
both
ways ?

Sergey
On 06/11/16 21:59, J. Fiala wrote:
Hi Sergey,

OK, I agree 100%, it would be great to have some progress
with
the PR
here if you have time for it.

Best regards,
Johannes


Am 06.11.2016 um 22:58 schrieb Sergey Beryozkin:
Hi Johannes

I'd say the WADL-related updates should be limited to
WADLGen/SourceGen

Cheers, Sergey
On 06/11/16 21:46, J. Fiala wrote:
Hi Sergey,

I can ask him or simply do a fork of the complete
project and
then
cover
it under the Apache license provided the author agrees with
this.

Do you think further BeanValidation extensions for CXF
should
also go
there and I should subclass the
WadlGenerator/SourceGenerator? Or
shall
we merge this at the Hackathon at ApacheCon maybe?

Best regards,
Johannes


Am 06.11.2016 um 22:40 schrieb Sergey Beryozkin:
Hi Johannes

Would it make sense asking the author to give you the
commit
rights to
the project ?
IMHO it can't go to the CXF master though perhaps an
option of
opening
a CXF sub-project is viable which will need to be
discussed on
the CXF
dev list. Whether it will stay where it is now or end up
as a
CXF
project, you'd have to support and release it, so I'm not
sure
what
difference would it make if it ended up being a CXF
sub-project
given
it already has a home...

Thanks, Sergey


On 06/11/16 19:32, Johannes Fiala wrote:
Hi there,

The JAXB annotation plugin provided by krasa
(https://github.com/krasa/krasa-jaxb-tools) is no longer
supported,
does
it make sense to contact the author to merge this into
CXF
master?

Maybe we could also get the BeanValidation for parameters
working
(https://github.com/apache/cxf/pull/146/files) to
improve the
overall
BeanValidation experience.

Is anybody interested in putting this on the agenda
for the
hackathon?

Best regards,
Johannes