Details

    • Type: New Feature New Feature
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      It should be possible to register jax resource classes and providers as services. As they don't implement a specific interface services that expose java.lang.Object should be considered as javx-rs services iff they have the service property "javax.ws.rs" set to true.

      1. slingr-on-wink-osgi.patch
        49 kB
        Reto Bachmann-Gmür
      2. SLING-2192-with-tests.patch
        50 kB
        Bertrand Delacretaz
      3. SLING-2192-with-sling-style-style-registration.patch
        45 kB
        Reto Bachmann-Gmür
      4. SLING-2192-new-jax-rs-bundle.patch
        9 kB
        Reto Bachmann-Gmür
      5. SLING-2192-new-jax-rs-bundle.patch
        16 kB
        Reto Bachmann-Gmür
      6. SLING-2192-20111013.patch
        53 kB
        Reto Bachmann-Gmür
      7. SLING-2192-20111004.patch
        55 kB
        Reto Bachmann-Gmür
      8. SLING-2192-20110310.patch
        55 kB
        Reto Bachmann-Gmür
      9. jaxrs-in-contrib.patch
        58 kB
        Reto Bachmann-Gmür

        Activity

        Hide
        Reto Bachmann-Gmür added a comment -

        This patch uses the newly release wink-osgi bundle. The created bulde is thus much smaller but to run it requires additionally the wink-osgi bundle.

        Show
        Reto Bachmann-Gmür added a comment - This patch uses the newly release wink-osgi bundle. The created bulde is thus much smaller but to run it requires additionally the wink-osgi bundle.
        Hide
        Reto Bachmann-Gmür added a comment -

        Finally a patch to add the jaxrs in contrib, also contains some version updates in the testing pom.

        Show
        Reto Bachmann-Gmür added a comment - Finally a patch to add the jaxrs in contrib, also contains some version updates in the testing pom.
        Hide
        Reto Bachmann-Gmür added a comment -

        SLING-2192-20111013.patch includes the tests and other requested improvements but it it not yet moved to contrib

        Show
        Reto Bachmann-Gmür added a comment - SLING-2192 -20111013.patch includes the tests and other requested improvements but it it not yet moved to contrib
        Hide
        Bertrand Delacretaz added a comment -

        Agreed, so that would be contrib/extensions/jaxrs, with integration tests under contrib/launchpad/testing

        Show
        Bertrand Delacretaz added a comment - Agreed, so that would be contrib/extensions/jaxrs, with integration tests under contrib/launchpad/testing
        Hide
        Carsten Ziegeler added a comment -

        The new bundle should go under /contrib (not /bundles) first.

        Show
        Carsten Ziegeler added a comment - The new bundle should go under /contrib (not /bundles) first.
        Hide
        Bertrand Delacretaz added a comment -

        Thanks for the updated patch, I still have a few comments:

        1) Are the RequestUriOptingServlet2Test.java and RequestUriOptingServlet2 tests useful with your latest patch? IIUC you switched to servlet filters so they're probably not needed, or should be part of a separate issue.

        2) The SimpleRootResourceTest of my SLING-2192-with-tests patch is missing

        3) The new bundle should be at bundles/extensions/jaxrs like in that patch

        4) I think we don't need the samples/slingrs, we'll create integration tests instead, that also act as code samples

        Show
        Bertrand Delacretaz added a comment - Thanks for the updated patch, I still have a few comments: 1) Are the RequestUriOptingServlet2Test.java and RequestUriOptingServlet2 tests useful with your latest patch? IIUC you switched to servlet filters so they're probably not needed, or should be part of a separate issue. 2) The SimpleRootResourceTest of my SLING-2192 -with-tests patch is missing 3) The new bundle should be at bundles/extensions/jaxrs like in that patch 4) I think we don't need the samples/slingrs, we'll create integration tests instead, that also act as code samples
        Hide
        Reto Bachmann-Gmür added a comment -

        With the previous patch registered a default wink resource for / was registered which conflicted with WebDav.

        Show
        Reto Bachmann-Gmür added a comment - With the previous patch registered a default wink resource for / was registered which conflicted with WebDav.
        Hide
        Reto Bachmann-Gmür added a comment -

        The timing issue aou mention under 3 is the motivation for SLING-2191, this is no longer an issue as the latest patch uses Servlet-Filter instead of OptingServlets.

        Show
        Reto Bachmann-Gmür added a comment - The timing issue aou mention under 3 is the motivation for SLING-2191 , this is no longer an issue as the latest patch uses Servlet-Filter instead of OptingServlets.
        Hide
        Reto Bachmann-Gmür added a comment -

        This patch incorporates the review comments 1 to 3. No new test is added as I wasn't sure against which resource type to register and test.

        Show
        Reto Bachmann-Gmür added a comment - This patch incorporates the review comments 1 to 3. No new test is added as I wasn't sure against which resource type to register and test.
        Hide
        Reto Bachmann-Gmür added a comment -

        Thanks for your review!

        In the patch for WINK-351 unregistration of resources is implemented. I will integrate this into a patch for this issue.

        There is currently a big code duplications between this issue and WINK-351, the idea of WINK-351 is to put the part that can be used both in Sling and Clerezza into wink. If the patch gets accepted for wink a future implementation of this feature in sling will require much less code.

        Show
        Reto Bachmann-Gmür added a comment - Thanks for your review! In the patch for WINK-351 unregistration of resources is implemented. I will integrate this into a patch for this issue. There is currently a big code duplications between this issue and WINK-351 , the idea of WINK-351 is to put the part that can be used both in Sling and Clerezza into wink. If the patch gets accepted for wink a future implementation of this feature in sling will require much less code.
        Hide
        Bertrand Delacretaz added a comment -

        Thanks for your contribution!

        Looks interesting, I tested it and made a few changes to your patch:
        -Moved bundle under extensions
        -Added the examples to the test-services instead of a separate sample so that they run as part of the build time integration tests.
        -Added an integration test, SimpleRootResourceTest

        I saw three issues in my tests:

        1) It seems like resources are not unregistered if you change their path: start with path A, install the bundle, change to path B in source code, install bundle, both A and B paths are active.

        2) More logging would be useful, in the SlingRSRegistrar and related classes.

        3) There seems to be an initialization timing issue, if starting a Sling instance with both jaxrs and test-services bundles installed sometimes the SimpleRootResource is not available. It becomes available after reinstalling the test-services bundle, so I assume the service detection is fragile w.r.t. installation/startup timing.

        Could you add more similar integration tests, including one with a resource that's registered by resource type? We want to discourage path-based registration in general.

        See launchpad/integration-tests/README.txt for how to run the integration tests individually agains a previously started instance, as they can take a few minutes when run in a full build.

        Show
        Bertrand Delacretaz added a comment - Thanks for your contribution! Looks interesting, I tested it and made a few changes to your patch: -Moved bundle under extensions -Added the examples to the test-services instead of a separate sample so that they run as part of the build time integration tests. -Added an integration test, SimpleRootResourceTest I saw three issues in my tests: 1) It seems like resources are not unregistered if you change their path: start with path A, install the bundle, change to path B in source code, install bundle, both A and B paths are active. 2) More logging would be useful, in the SlingRSRegistrar and related classes. 3) There seems to be an initialization timing issue, if starting a Sling instance with both jaxrs and test-services bundles installed sometimes the SimpleRootResource is not available. It becomes available after reinstalling the test-services bundle, so I assume the service detection is fragile w.r.t. installation/startup timing. Could you add more similar integration tests, including one with a resource that's registered by resource type? We want to discourage path-based registration in general. See launchpad/integration-tests/README.txt for how to run the integration tests individually agains a previously started instance, as they can take a few minutes when run in a full build.
        Hide
        Reto Bachmann-Gmür added a comment -

        Added a patch that adds support to "slingrs": jax-rs style resources that use jax-features such argument injection and response processor. path contain implementation and demo bundle.

        Resources classes with the property sling.ws.rs set to true are bound the sling way with the same annotations as the equivalent servlet. In the thread at http://mail-archives.apache.org/mod_mbox/sling-dev/201108.mbox/%3CCAEWfVJ=TwX1cNoNpSCQ198vYfyspaTKwy_v9Ufxj4Nq=Ff70MQ@mail.gmail.com%3E supporting jax-rs has been seen as concurring with the sling way of doing things, this patch shows how one can have sling-style binding and jax-rs features that make writing application easier.

        Show
        Reto Bachmann-Gmür added a comment - Added a patch that adds support to "slingrs": jax-rs style resources that use jax-features such argument injection and response processor. path contain implementation and demo bundle. Resources classes with the property sling.ws.rs set to true are bound the sling way with the same annotations as the equivalent servlet. In the thread at http://mail-archives.apache.org/mod_mbox/sling-dev/201108.mbox/%3CCAEWfVJ=TwX1cNoNpSCQ198vYfyspaTKwy_v9Ufxj4Nq=Ff70MQ@mail.gmail.com%3E supporting jax-rs has been seen as concurring with the sling way of doing things, this patch shows how one can have sling-style binding and jax-rs features that make writing application easier.
        Hide
        Reto Bachmann-Gmür added a comment -

        added missing files. Thanks Bertrand.

        Show
        Reto Bachmann-Gmür added a comment - added missing files. Thanks Bertrand.
        Hide
        Bertrand Delacretaz added a comment -

        Note that the pom.xml is missing from your patch. I'll also discuss the JaxRsServlet servlet registration on the dev list.

        Show
        Bertrand Delacretaz added a comment - Note that the pom.xml is missing from your patch. I'll also discuss the JaxRsServlet servlet registration on the dev list.
        Hide
        Reto Bachmann-Gmür added a comment -

        new bundle adding jax-rs resources support using apache wink.
        Missing:

        • Tests (need resoution of SLING-2191)
        • unregistrations of components/providers
        Show
        Reto Bachmann-Gmür added a comment - new bundle adding jax-rs resources support using apache wink. Missing: Tests (need resoution of SLING-2191 ) unregistrations of components/providers

          People

          • Assignee:
            Unassigned
            Reporter:
            Reto Bachmann-Gmür
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Development