Uploaded image for project: 'CXF'
  1. CXF
  2. CXF-7657

Reader and Writer interceptors are not sorted by the provider comparator

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 3.2.2, 3.2.3
    • 3.2.4
    • JAX-RS
    • None
    • Unknown
    • Patch

    Description

      ReaderInterceptors and WriterInterceptors are not sortered when a provider comparator is provided.

      Please see attached patch for a proposed (probably excessively naive) solution.

      Attachments

        1. patch.diff
          4 kB
          Carlos Sierra Andrés

        Issue Links

          Activity

            githubbot ASF GitHub Bot added a comment -

            csierra opened a new pull request #385: CXF-7657 Reader and Writer interceptors are not sorted by the provi…
            URL: https://github.com/apache/cxf/pull/385

            …der comparator

            ----------------------------------------------------------------
            This is an automated message from the Apache Git Service.
            To respond to the message, please log on GitHub and use the
            URL above to go to the specific comment.

            For queries about this service, please contact Infrastructure at:
            users@infra.apache.org

            githubbot ASF GitHub Bot added a comment - csierra opened a new pull request #385: CXF-7657 Reader and Writer interceptors are not sorted by the provi… URL: https://github.com/apache/cxf/pull/385 …der comparator ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: users@infra.apache.org
            githubbot ASF GitHub Bot added a comment -

            csierra commented on issue #385: CXF-7657 Reader and Writer interceptors are not sorted by the provi…
            URL: https://github.com/apache/cxf/pull/385#issuecomment-368851321

            hi @andymc12,

            I can try but the problem is that the failures are random because the fallback case when the providers come with the same `@Priority` is ```Integer.compare(o1.hashCode(), o2.hashCode())```

            ----------------------------------------------------------------
            This is an automated message from the Apache Git Service.
            To respond to the message, please log on GitHub and use the
            URL above to go to the specific comment.

            For queries about this service, please contact Infrastructure at:
            users@infra.apache.org

            githubbot ASF GitHub Bot added a comment - csierra commented on issue #385: CXF-7657 Reader and Writer interceptors are not sorted by the provi… URL: https://github.com/apache/cxf/pull/385#issuecomment-368851321 hi @andymc12, I can try but the problem is that the failures are random because the fallback case when the providers come with the same `@Priority` is ```Integer.compare(o1.hashCode(), o2.hashCode())``` ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: users@infra.apache.org
            githubbot ASF GitHub Bot added a comment -

            csierra commented on issue #385: CXF-7657 Reader and Writer interceptors are not sorted by the provi…
            URL: https://github.com/apache/cxf/pull/385#issuecomment-371901800

            hey @andymc12,

            sorry this took so long. I updated the test showing the problem with the ReaderInterceptor and WriterInterceptor and the `Comparator`. If you revert the fix you will see the test failing. There is a small chance, though, that the test passes because the current behaviour is random, so it might happen that the writers fall in the expected order even though they are not being sorted.

            ----------------------------------------------------------------
            This is an automated message from the Apache Git Service.
            To respond to the message, please log on GitHub and use the
            URL above to go to the specific comment.

            For queries about this service, please contact Infrastructure at:
            users@infra.apache.org

            githubbot ASF GitHub Bot added a comment - csierra commented on issue #385: CXF-7657 Reader and Writer interceptors are not sorted by the provi… URL: https://github.com/apache/cxf/pull/385#issuecomment-371901800 hey @andymc12, sorry this took so long. I updated the test showing the problem with the ReaderInterceptor and WriterInterceptor and the `Comparator`. If you revert the fix you will see the test failing. There is a small chance, though, that the test passes because the current behaviour is random, so it might happen that the writers fall in the expected order even though they are not being sorted. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: users@infra.apache.org
            githubbot ASF GitHub Bot added a comment -

            andymc12 closed pull request #385: CXF-7657 Reader and Writer interceptors are not sorted by the provi…
            URL: https://github.com/apache/cxf/pull/385

            This is a PR merged from a forked repository.
            As GitHub hides the original diff on merge, it is displayed below for
            the sake of provenance:

            As this is a foreign pull request (from a fork), the diff is supplied
            below (as it won't show otherwise due to GitHub magic):

            diff --git a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/provider/ProviderFactory.java b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/provider/ProviderFactory.java
            index a5767a0c165..9f2baeba90b 100644
            — a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/provider/ProviderFactory.java
            +++ b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/provider/ProviderFactory.java
            @@ -1212,12 +1212,24 @@ public T getContext(Class<?> cls) {
            private String name;
            private Integer priority;
            private Class<?> providerCls;
            + private ProviderInfo<?> providerInfo;
            +
            public NameKey(String name,
            int priority,
            Class<?> providerCls)

            { + + this(name, priority, providerCls, null); + }

            +
            + public NameKey(String name,
            + int priority,
            + Class<?> providerCls,
            + ProviderInfo<?> provider)

            { + this.name = name; this.priority = priority; this.providerCls = providerCls; + this.providerInfo = provider; }

            public String getName() {
            @@ -1228,6 +1240,10 @@ public Integer getPriority()

            { return priority; }

            + public ProviderInfo<?> getProviderInfo()

            { + return providerInfo; + }

            +
            public boolean equals(Object o) {
            if (!(o instanceof NameKey)) {
            return false;
            @@ -1257,7 +1273,7 @@ public String toString() {
            int priority = getFilterPriority(p, providerCls);

            for (String name : names)

            { - map.put(new NameKey(name, priority, p.getClass()), p); + map.put(new NameKey(name, priority, p.getClass(), p), p); }

            }

            @@ -1288,8 +1304,17 @@ protected static int getFilterPriority(ProviderInfo<?> p, Class<?> providerCls)
            protected static class NameKeyComparator extends AbstractPriorityComparator
            implements Comparator<NameKey> {

            + private final Comparator<ProviderInfo<?>> comparator;
            +
            public NameKeyComparator(boolean ascending)

            { + this(null, ascending); + }

            +
            + public NameKeyComparator(
            + Comparator<ProviderInfo<?>> comparator, boolean ascending)

            { + super(ascending); + this.comparator = comparator; }

            @Override
            @@ -1298,14 +1323,29 @@ public int compare(NameKey key1, NameKey key2) {
            if (result != 0)

            { return result; }

            +
            + if (comparator != null) {
            + result = comparator.compare(
            + key1.getProviderInfo(), key2.getProviderInfo());
            +
            + if (result != 0)

            { + return result; + }

            + }
            +
            return compare(key1.hashCode(), key2.hashCode());
            }
            -
            }

            protected static class NameKeyMap<T> extends TreeMap<NameKey, T> {
            private static final long serialVersionUID = -4352258671270502204L;

            + public NameKeyMap(
            + Comparator<ProviderInfo<?>> comparator, boolean ascending)

            { + + super(new NameKeyComparator(comparator, ascending)); + }

            +
            public NameKeyMap(boolean ascending)

            { super(new NameKeyComparator(ascending)); }

            @@ -1371,8 +1411,21 @@ protected static boolean filterContractSupported(ProviderInfo<?> provider,

            public void setProviderComparator(Comparator<?> providerComparator)

            { this.providerComparator = providerComparator; + sortReaders(); sortWriters(); + + NameKeyMap<ProviderInfo<ReaderInterceptor>> sortedReaderInterceptors = + new NameKeyMap<>( + (Comparator<ProviderInfo<?>>) providerComparator, true); + sortedReaderInterceptors.putAll(readerInterceptors); + NameKeyMap<ProviderInfo<WriterInterceptor>> sortedWriterInterceptors = + new NameKeyMap<>( + (Comparator<ProviderInfo<?>>) providerComparator, true); + sortedWriterInterceptors.putAll(writerInterceptors); + + readerInterceptors = sortedReaderInterceptors; + writerInterceptors = sortedWriterInterceptors; }

            }
            diff --git a/rt/frontend/jaxrs/src/test/java/org/apache/cxf/jaxrs/provider/ProviderFactoryTest.java b/rt/frontend/jaxrs/src/test/java/org/apache/cxf/jaxrs/provider/ProviderFactoryTest.java
            index e34c4597d5c..0878870b8f0 100644
            — a/rt/frontend/jaxrs/src/test/java/org/apache/cxf/jaxrs/provider/ProviderFactoryTest.java
            +++ b/rt/frontend/jaxrs/src/test/java/org/apache/cxf/jaxrs/provider/ProviderFactoryTest.java
            @@ -26,8 +26,11 @@
            import java.lang.annotation.Annotation;
            import java.lang.reflect.Type;
            import java.util.ArrayList;
            +import java.util.Arrays;
            +import java.util.Collection;
            import java.util.Collections;
            import java.util.Comparator;
            +import java.util.Iterator;
            import java.util.List;
            import java.util.Map;

            @@ -49,6 +52,8 @@
            import javax.ws.rs.ext.MessageBodyWriter;
            import javax.ws.rs.ext.ParamConverter;
            import javax.ws.rs.ext.ParamConverterProvider;
            +import javax.ws.rs.ext.WriterInterceptor;
            +import javax.ws.rs.ext.WriterInterceptorContext;
            import javax.xml.bind.JAXBContext;
            import javax.xml.bind.annotation.XmlRootElement;
            import javax.xml.validation.Schema;
            @@ -299,6 +304,79 @@ public int compare(ProviderInfo<MessageBodyWriter<?>> o1, ProviderInfo<MessageBo
            assertFalse(lastReader instanceof StringTextProvider);
            }

            + @Test
            + public void testCustomProviderSortingWIOnly() {
            + ProviderFactory pf = ServerProviderFactory.getInstance();
            +
            + pf.setUserProviders(
            + Arrays.asList(
            + new DWriterInterceptor(), new CWriterInterceptor(),
            + new AWriterInterceptor(), new BWriterInterceptor()));
            +
            + Comparator<ProviderInfo<WriterInterceptor>> comp =
            + new Comparator<ProviderInfo<WriterInterceptor>>() {
            +
            + @Override
            + public int compare(
            + ProviderInfo<WriterInterceptor> o1,
            + ProviderInfo<WriterInterceptor> o2)

            { + + WriterInterceptor provider1 = o1.getProvider(); + WriterInterceptor provider2 = o2.getProvider(); + + return provider1.getClass().getName().compareTo( + provider2.getClass().getName()); + }

            +
            + };
            +
            + pf.setProviderComparator(comp);
            +
            + Collection<ProviderInfo<WriterInterceptor>> values =
            + pf.writerInterceptors.values();
            +
            + assertEquals(4, values.size());
            +
            + Iterator<ProviderInfo<WriterInterceptor>> iterator = values.iterator();
            +
            + assertEquals(AWriterInterceptor.class, iterator.next().getProvider().getClass());
            + assertEquals(BWriterInterceptor.class, iterator.next().getProvider().getClass());
            + assertEquals(CWriterInterceptor.class, iterator.next().getProvider().getClass());
            + assertEquals(DWriterInterceptor.class, iterator.next().getProvider().getClass());
            + }
            +
            + private static class AWriterInterceptor implements WriterInterceptor {
            + @Override
            + public void aroundWriteTo(WriterInterceptorContext context) throws
            + IOException, WebApplicationException

            { + + }
            + }
            +
            + private static class BWriterInterceptor implements WriterInterceptor {
            + @Override
            + public void aroundWriteTo(WriterInterceptorContext context) throws
            + IOException, WebApplicationException {++ }

            + }
            +
            + private static class CWriterInterceptor implements WriterInterceptor {
            + @Override
            + public void aroundWriteTo(WriterInterceptorContext context) throws
            + IOException, WebApplicationException

            { + + }
            + }
            +
            + private static class DWriterInterceptor implements WriterInterceptor {
            + @Override
            + public void aroundWriteTo(WriterInterceptorContext context) throws
            + IOException, WebApplicationException {++ }

            + }
            +
            @Test
            public void testCustomProviderSortingWithBus() {
            WildcardReader wc1 = new WildcardReader();

            ----------------------------------------------------------------
            This is an automated message from the Apache Git Service.
            To respond to the message, please log on GitHub and use the
            URL above to go to the specific comment.

            For queries about this service, please contact Infrastructure at:
            users@infra.apache.org

            githubbot ASF GitHub Bot added a comment - andymc12 closed pull request #385: CXF-7657 Reader and Writer interceptors are not sorted by the provi… URL: https://github.com/apache/cxf/pull/385 This is a PR merged from a forked repository. As GitHub hides the original diff on merge, it is displayed below for the sake of provenance: As this is a foreign pull request (from a fork), the diff is supplied below (as it won't show otherwise due to GitHub magic): diff --git a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/provider/ProviderFactory.java b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/provider/ProviderFactory.java index a5767a0c165..9f2baeba90b 100644 — a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/provider/ProviderFactory.java +++ b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/provider/ProviderFactory.java @@ -1212,12 +1212,24 @@ public T getContext(Class<?> cls) { private String name; private Integer priority; private Class<?> providerCls; + private ProviderInfo<?> providerInfo; + public NameKey(String name, int priority, Class<?> providerCls) { + + this(name, priority, providerCls, null); + } + + public NameKey(String name, + int priority, + Class<?> providerCls, + ProviderInfo<?> provider) { + this.name = name; this.priority = priority; this.providerCls = providerCls; + this.providerInfo = provider; } public String getName() { @@ -1228,6 +1240,10 @@ public Integer getPriority() { return priority; } + public ProviderInfo<?> getProviderInfo() { + return providerInfo; + } + public boolean equals(Object o) { if (!(o instanceof NameKey)) { return false; @@ -1257,7 +1273,7 @@ public String toString() { int priority = getFilterPriority(p, providerCls); for (String name : names) { - map.put(new NameKey(name, priority, p.getClass()), p); + map.put(new NameKey(name, priority, p.getClass(), p), p); } } @@ -1288,8 +1304,17 @@ protected static int getFilterPriority(ProviderInfo<?> p, Class<?> providerCls) protected static class NameKeyComparator extends AbstractPriorityComparator implements Comparator<NameKey> { + private final Comparator<ProviderInfo<?>> comparator; + public NameKeyComparator(boolean ascending) { + this(null, ascending); + } + + public NameKeyComparator( + Comparator<ProviderInfo<?>> comparator, boolean ascending) { + super(ascending); + this.comparator = comparator; } @Override @@ -1298,14 +1323,29 @@ public int compare(NameKey key1, NameKey key2) { if (result != 0) { return result; } + + if (comparator != null) { + result = comparator.compare( + key1.getProviderInfo(), key2.getProviderInfo()); + + if (result != 0) { + return result; + } + } + return compare(key1.hashCode(), key2.hashCode()); } - } protected static class NameKeyMap<T> extends TreeMap<NameKey, T> { private static final long serialVersionUID = -4352258671270502204L; + public NameKeyMap( + Comparator<ProviderInfo<?>> comparator, boolean ascending) { + + super(new NameKeyComparator(comparator, ascending)); + } + public NameKeyMap(boolean ascending) { super(new NameKeyComparator(ascending)); } @@ -1371,8 +1411,21 @@ protected static boolean filterContractSupported(ProviderInfo<?> provider, public void setProviderComparator(Comparator<?> providerComparator) { this.providerComparator = providerComparator; + sortReaders(); sortWriters(); + + NameKeyMap<ProviderInfo<ReaderInterceptor>> sortedReaderInterceptors = + new NameKeyMap<>( + (Comparator<ProviderInfo<?>>) providerComparator, true); + sortedReaderInterceptors.putAll(readerInterceptors); + NameKeyMap<ProviderInfo<WriterInterceptor>> sortedWriterInterceptors = + new NameKeyMap<>( + (Comparator<ProviderInfo<?>>) providerComparator, true); + sortedWriterInterceptors.putAll(writerInterceptors); + + readerInterceptors = sortedReaderInterceptors; + writerInterceptors = sortedWriterInterceptors; } } diff --git a/rt/frontend/jaxrs/src/test/java/org/apache/cxf/jaxrs/provider/ProviderFactoryTest.java b/rt/frontend/jaxrs/src/test/java/org/apache/cxf/jaxrs/provider/ProviderFactoryTest.java index e34c4597d5c..0878870b8f0 100644 — a/rt/frontend/jaxrs/src/test/java/org/apache/cxf/jaxrs/provider/ProviderFactoryTest.java +++ b/rt/frontend/jaxrs/src/test/java/org/apache/cxf/jaxrs/provider/ProviderFactoryTest.java @@ -26,8 +26,11 @@ import java.lang.annotation.Annotation; import java.lang.reflect.Type; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.Iterator; import java.util.List; import java.util.Map; @@ -49,6 +52,8 @@ import javax.ws.rs.ext.MessageBodyWriter; import javax.ws.rs.ext.ParamConverter; import javax.ws.rs.ext.ParamConverterProvider; +import javax.ws.rs.ext.WriterInterceptor; +import javax.ws.rs.ext.WriterInterceptorContext; import javax.xml.bind.JAXBContext; import javax.xml.bind.annotation.XmlRootElement; import javax.xml.validation.Schema; @@ -299,6 +304,79 @@ public int compare(ProviderInfo<MessageBodyWriter<?>> o1, ProviderInfo<MessageBo assertFalse(lastReader instanceof StringTextProvider); } + @Test + public void testCustomProviderSortingWIOnly() { + ProviderFactory pf = ServerProviderFactory.getInstance(); + + pf.setUserProviders( + Arrays.asList( + new DWriterInterceptor(), new CWriterInterceptor(), + new AWriterInterceptor(), new BWriterInterceptor())); + + Comparator<ProviderInfo<WriterInterceptor>> comp = + new Comparator<ProviderInfo<WriterInterceptor>>() { + + @Override + public int compare( + ProviderInfo<WriterInterceptor> o1, + ProviderInfo<WriterInterceptor> o2) { + + WriterInterceptor provider1 = o1.getProvider(); + WriterInterceptor provider2 = o2.getProvider(); + + return provider1.getClass().getName().compareTo( + provider2.getClass().getName()); + } + + }; + + pf.setProviderComparator(comp); + + Collection<ProviderInfo<WriterInterceptor>> values = + pf.writerInterceptors.values(); + + assertEquals(4, values.size()); + + Iterator<ProviderInfo<WriterInterceptor>> iterator = values.iterator(); + + assertEquals(AWriterInterceptor.class, iterator.next().getProvider().getClass()); + assertEquals(BWriterInterceptor.class, iterator.next().getProvider().getClass()); + assertEquals(CWriterInterceptor.class, iterator.next().getProvider().getClass()); + assertEquals(DWriterInterceptor.class, iterator.next().getProvider().getClass()); + } + + private static class AWriterInterceptor implements WriterInterceptor { + @Override + public void aroundWriteTo(WriterInterceptorContext context) throws + IOException, WebApplicationException { + + } + } + + private static class BWriterInterceptor implements WriterInterceptor { + @Override + public void aroundWriteTo(WriterInterceptorContext context) throws + IOException, WebApplicationException {++ } + } + + private static class CWriterInterceptor implements WriterInterceptor { + @Override + public void aroundWriteTo(WriterInterceptorContext context) throws + IOException, WebApplicationException { + + } + } + + private static class DWriterInterceptor implements WriterInterceptor { + @Override + public void aroundWriteTo(WriterInterceptorContext context) throws + IOException, WebApplicationException {++ } + } + @Test public void testCustomProviderSortingWithBus() { WildcardReader wc1 = new WildcardReader(); ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: users@infra.apache.org

            People

              Unassigned Unassigned
              csierra Carlos Sierra Andrés
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: