https://issues.apache.org/jira/browse/TOMEE-2100 - @Priority

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

https://issues.apache.org/jira/browse/TOMEE-2100 - @Priority

jgallimore
I'm happy to take a look at this one, unless someone else is already
working on it?

Jon
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: https://issues.apache.org/jira/browse/TOMEE-2100 - @Priority

Romain Manni-Bucau
+1

What's do we want to do? Looks like a EE 8 feature we can eagerly support
as a default comparator (if the comparator is set we must bypass it to
respect the comparator IMHO)


Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> |  Blog
<https://blog-rmannibucau.rhcloud.com> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
<https://javaeefactory-rmannibucau.rhcloud.com>

2017-07-25 17:03 GMT+02:00 Jonathan Gallimore <[hidden email]>
:

> I'm happy to take a look at this one, unless someone else is already
> working on it?
>
> Jon
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: https://issues.apache.org/jira/browse/TOMEE-2100 - @Priority

jgallimore
Yep - that's what I had in my mind. Makes sense if a comparator is
specified, we should use that instead of the @Priority ordering.

Jon

On Tue, Jul 25, 2017 at 4:59 PM, Romain Manni-Bucau <[hidden email]>
wrote:

> +1
>
> What's do we want to do? Looks like a EE 8 feature we can eagerly support
> as a default comparator (if the comparator is set we must bypass it to
> respect the comparator IMHO)
>
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> <https://blog-rmannibucau.rhcloud.com> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <https://github.com/
> rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
> <https://javaeefactory-rmannibucau.rhcloud.com>
>
> 2017-07-25 17:03 GMT+02:00 Jonathan Gallimore <
> [hidden email]>
> :
>
> > I'm happy to take a look at this one, unless someone else is already
> > working on it?
> >
> > Jon
> >
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: https://issues.apache.org/jira/browse/TOMEE-2100 - @Priority

jgallimore
Hi,

I've had a go at this: https://github.com/apache/tomee/pull/96

This creates a default comparator when no setting for
cxf.jaxrs.provider-comparator is specified. This will order by priority
first, but will still incorporate the default CXF sorting where priorities
are the same for backwards compatibility.

Michel's test case appears to work ok with this patch.

Any feedback would be most welcome.

Many thanks

Jon

On Tue, Jul 25, 2017 at 5:02 PM, Jonathan Gallimore <
[hidden email]> wrote:

> Yep - that's what I had in my mind. Makes sense if a comparator is
> specified, we should use that instead of the @Priority ordering.
>
> Jon
>
> On Tue, Jul 25, 2017 at 4:59 PM, Romain Manni-Bucau <[hidden email]
> > wrote:
>
>> +1
>>
>> What's do we want to do? Looks like a EE 8 feature we can eagerly support
>> as a default comparator (if the comparator is set we must bypass it to
>> respect the comparator IMHO)
>>
>>
>> Romain Manni-Bucau
>> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
>> <https://blog-rmannibucau.rhcloud.com> | Old Blog
>> <http://rmannibucau.wordpress.com> | Github <
>> https://github.com/rmannibucau> |
>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
>> <https://javaeefactory-rmannibucau.rhcloud.com>
>>
>> 2017-07-25 17:03 GMT+02:00 Jonathan Gallimore <
>> [hidden email]>
>> :
>>
>> > I'm happy to take a look at this one, unless someone else is already
>> > working on it?
>> >
>> > Jon
>> >
>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: https://issues.apache.org/jira/browse/TOMEE-2100 - @Priority

Romain Manni-Bucau
Hi Jon

If we do it this way - jaxrs 2.1 - I d be to PR cxf instead of tomee. We
fork too much code to stay sane here.

What about just sorting by priority blindly? Cxf will handle the q and type
sorting anyway, no?

Le 26 juil. 2017 22:18, "Jonathan Gallimore" <[hidden email]>
a écrit :

> Hi,
>
> I've had a go at this: https://github.com/apache/tomee/pull/96
>
> This creates a default comparator when no setting for
> cxf.jaxrs.provider-comparator is specified. This will order by priority
> first, but will still incorporate the default CXF sorting where priorities
> are the same for backwards compatibility.
>
> Michel's test case appears to work ok with this patch.
>
> Any feedback would be most welcome.
>
> Many thanks
>
> Jon
>
> On Tue, Jul 25, 2017 at 5:02 PM, Jonathan Gallimore <
> [hidden email]> wrote:
>
> > Yep - that's what I had in my mind. Makes sense if a comparator is
> > specified, we should use that instead of the @Priority ordering.
> >
> > Jon
> >
> > On Tue, Jul 25, 2017 at 4:59 PM, Romain Manni-Bucau <
> [hidden email]
> > > wrote:
> >
> >> +1
> >>
> >> What's do we want to do? Looks like a EE 8 feature we can eagerly
> support
> >> as a default comparator (if the comparator is set we must bypass it to
> >> respect the comparator IMHO)
> >>
> >>
> >> Romain Manni-Bucau
> >> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> >> <https://blog-rmannibucau.rhcloud.com> | Old Blog
> >> <http://rmannibucau.wordpress.com> | Github <
> >> https://github.com/rmannibucau> |
> >> LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
> >> <https://javaeefactory-rmannibucau.rhcloud.com>
> >>
> >> 2017-07-25 17:03 GMT+02:00 Jonathan Gallimore <
> >> [hidden email]>
> >> :
> >>
> >> > I'm happy to take a look at this one, unless someone else is already
> >> > working on it?
> >> >
> >> > Jon
> >> >
> >>
> >
> >
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: https://issues.apache.org/jira/browse/TOMEE-2100 - @Priority

jgallimore
I'm inclined to agree, but glad I proposed the PR. Its a shame that
the org.apache.cxf.jaxrs.provider.ProviderFactory.MessageBodyReaderComparator
and org.apache.cxf.jaxrs.provider.ProviderFactory.MessageBodyWriterComparator
in CXF are private, otherwise we could most likely call those directly.

I did try a straight comparator on the @Priority value directly, which did
work for Michel's case, but lead to a number of test failures across the
openejb-cxf-rs module, the worst of which seemed to be related to malformed
JSON in the Johnzon provider due to things happening in the wrong order.

Doing this (ordering by class name as well as priority), the results are
less bad:

private static final class PriorityProviderComparator implements
Comparator<ProviderInfo<?>> {

        @Override
        public int compare(final ProviderInfo<?> o1, final ProviderInfo<?>
o2) {

            final int p1 = getPriority(o1.getProvider().getClass());
            final int p2 = getPriority(o2.getProvider().getClass());

            final int compare = Integer.compare(p1, p2);

            if (compare != 0) {
                return compare;
            }

            return
o1.getProvider().getClass().getName().compareTo(o2.getProvider().getClass().getName());
        }
}

[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR]   AdvancedProviderConfigTest.check:103 expected:<[tru]e> but
was:<[fals]e>
[ERROR]   CDIProviderTest.isCdi:74 expected:<[Oh Yeah!]> but was:<[failed]>
[ERROR]   EJBProviderTest.isEJB:66 expected:<[Oh Yeah!]> but was:<[failed]>
[INFO]
[ERROR] Tests run: 106, Failures: 3, Errors: 0, Skipped: 1

I suspect this is more down to luck as opposed to being a good solution
though.

Michel - I'll have a go at pushing a PR to CXF for this one this evening.

Cheers

Jon

On Thu, Jul 27, 2017 at 5:39 AM, Romain Manni-Bucau <[hidden email]>
wrote:

> Hi Jon
>
> If we do it this way - jaxrs 2.1 - I d be to PR cxf instead of tomee. We
> fork too much code to stay sane here.
>
> What about just sorting by priority blindly? Cxf will handle the q and type
> sorting anyway, no?
>
> Le 26 juil. 2017 22:18, "Jonathan Gallimore" <[hidden email]
> >
> a écrit :
>
> > Hi,
> >
> > I've had a go at this: https://github.com/apache/tomee/pull/96
> >
> > This creates a default comparator when no setting for
> > cxf.jaxrs.provider-comparator is specified. This will order by priority
> > first, but will still incorporate the default CXF sorting where
> priorities
> > are the same for backwards compatibility.
> >
> > Michel's test case appears to work ok with this patch.
> >
> > Any feedback would be most welcome.
> >
> > Many thanks
> >
> > Jon
> >
> > On Tue, Jul 25, 2017 at 5:02 PM, Jonathan Gallimore <
> > [hidden email]> wrote:
> >
> > > Yep - that's what I had in my mind. Makes sense if a comparator is
> > > specified, we should use that instead of the @Priority ordering.
> > >
> > > Jon
> > >
> > > On Tue, Jul 25, 2017 at 4:59 PM, Romain Manni-Bucau <
> > [hidden email]
> > > > wrote:
> > >
> > >> +1
> > >>
> > >> What's do we want to do? Looks like a EE 8 feature we can eagerly
> > support
> > >> as a default comparator (if the comparator is set we must bypass it to
> > >> respect the comparator IMHO)
> > >>
> > >>
> > >> Romain Manni-Bucau
> > >> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> > >> <https://blog-rmannibucau.rhcloud.com> | Old Blog
> > >> <http://rmannibucau.wordpress.com> | Github <
> > >> https://github.com/rmannibucau> |
> > >> LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
> > >> <https://javaeefactory-rmannibucau.rhcloud.com>
> > >>
> > >> 2017-07-25 17:03 GMT+02:00 Jonathan Gallimore <
> > >> [hidden email]>
> > >> :
> > >>
> > >> > I'm happy to take a look at this one, unless someone else is already
> > >> > working on it?
> > >> >
> > >> > Jon
> > >> >
> > >>
> > >
> > >
> >
>
Loading...