GeoServer
  1. GeoServer
  2. GEOS-4753

RESTConfig doesn't calculate Coverage grid/dimensions

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.1
    • Fix Version/s: 2.1.3, 2.2.x
    • Component/s: Configuration, REST
    • Labels:
      None
    • Testcase included:
      yes
    • Patch Submitted:
      Yes
    • Number of attachments :
      9

      Description

      Expanding on GEOS-4596, 'grid' and 'dimensions' are also not calculated for Coverages created via the REST API. And WCS DescribeCoverage/etc all explode when grid/dimensions/etc aren't set.

      Collections like supportedFormats/dimensions/etc are set to null rather than the default empty List from CoverageInfoImpl if they're not specified in the POST data. This happens because XStreamPersister doesn't run the constructor when unmarshalling - the object gets magicked up with no attributes setup.

      The attached patch:

      • makes ResourceInfoConverter, FeatureTypeInfoConverter, and CoverageInfoConverter setup empty lists on unmarshalling the same way the default implementation constructors do. So not specifying eg. supportedFormats will leave your coverageInfo object with an empty list rather than null - the same as if you instantiated a CoverageInfoImpl object directly.
      • move most of the logic from CatalogBuiler.buildCoverage() (called via the Web UI) to CatalogBuilder.initCoverage() (was called by REST after GEOS-4596, now by both). Wrap the calculations/derivations so they only happen if the values aren't set already. This allows you to override via REST, but the defaults will now be sensible - the same as if you created the coverage via the Web UI.
      • enable CoverageTest.testPostAsJSON() again, and set it up to use the same data as testPostAsXML() - but auto-specify nothing so it's all calculated automatically, then assert against the calculated values.

      rest_coverageinfo.contentOnly.patch has no whitespace changes included for easier reviewing, and rest_coverageinfo.patch has everything formatted with the GeoTools style.

      1. geos-4753-coverageinfo.patch
        27 kB
        Tony Young
      2. geos-4753-coverageinfo-v2.patch
        9 kB
        Tony Young
      3. geos-4753-coverageinfo-v3.patch
        11 kB
        Tony Young
      4. geos-4753-coverageinfo-v3.patch
        11 kB
        Tony Young
      5. geos-4753-coverageinfo-v4.patch
        3 kB
        Tony Young
      6. geos-4753-coverageinfo-v5.patch
        2 kB
        Tony Young
      7. geos-4753-coverageinfo-v5.patch
        2 kB
        Tony Young
      8. rest_coverageinfo.contentOnly.patch
        24 kB
        Robert Coup
      9. rest_coverageinfo.patch
        29 kB
        Robert Coup

        Activity

        Hide
        Robert Coup added a comment -

        Patches are against the 2.1.x branch

        Show
        Robert Coup added a comment - Patches are against the 2.1.x branch
        Hide
        Justin Deoliveira added a comment -

        Hi Robert, thanks for the patch. I have some comments/reservations.

        About initializing all the collectios to empty upon unmarshaling. I am a bit hesitant about this one for a couple of reasons. First I think it will throw off some of the logic in CatalogBuilder.update(). Basically if collections from unmarshalled objects are coming back as empty and not null, then during PUT operations where the user does not actually specify that collection at all, the target object will have its collection wiped out and replaced with the empty one.

        Second this duplicates logic already present in the catalog. The catalog should be resolving these collections when the object is added... so if a service is seeing a null collection it is probably a bug in the CatalogImpl.resolve() methods, where just a collection is being missed or something.

        Show
        Justin Deoliveira added a comment - Hi Robert, thanks for the patch. I have some comments/reservations. About initializing all the collectios to empty upon unmarshaling. I am a bit hesitant about this one for a couple of reasons. First I think it will throw off some of the logic in CatalogBuilder.update(). Basically if collections from unmarshalled objects are coming back as empty and not null, then during PUT operations where the user does not actually specify that collection at all, the target object will have its collection wiped out and replaced with the empty one. Second this duplicates logic already present in the catalog. The catalog should be resolving these collections when the object is added... so if a service is seeing a null collection it is probably a bug in the CatalogImpl.resolve() methods, where just a collection is being missed or something.
        Hide
        Tony Young added a comment -

        Hi, here's a patch for the current trunk (r16592). It incorporates some of rest_coverageinfo.patch and introduces setter methods for CoverageInfo and ResourceInfo instead of creating lists in the unmarshalling process.

        I'm not sure about the logic duplication part, could you elaborate?

        Show
        Tony Young added a comment - Hi, here's a patch for the current trunk (r16592). It incorporates some of rest_coverageinfo.patch and introduces setter methods for CoverageInfo and ResourceInfo instead of creating lists in the unmarshalling process. I'm not sure about the logic duplication part, could you elaborate?
        Hide
        Justin Deoliveira added a comment -

        Hi Tony,

        Regarding the modifications to the ResourceInfo to add setters for collections. We don't want to do this since we don't want the collections settable by client code. We want to hide as much as we can a collection implementation from client code, for example, consider storing catalog objects in hibernate or some ORM. They actually use their own lazy collections of sorts, allowing client code to set them would break this scheme. Now having the Impl classes have the setters for the collecitons, that is ok.

        I understand this is a problem for the rest api since the xstream depersister returns back objects that have null collections. For the most part this is handled in the catalog... look at catalog.add for the various object types and you will see code that "resolves" the collections, ie, sets them by casting to the Impl class. Now again I realize this doesn't help in the rest config case because again, the object has not been added to the catalog. But i would prefer that CatalogBuilder take the same approach, and simply cast to the Impl class.

        Regarding the changes to CatalogBuilder, seems there are a lot of them. Can you provide a more detailed description of what was changed. Also, please ensure that only the minimum changes are there, it seems there is more in the patch than just calculating the coverage dimensions.

        Show
        Justin Deoliveira added a comment - Hi Tony, Regarding the modifications to the ResourceInfo to add setters for collections. We don't want to do this since we don't want the collections settable by client code. We want to hide as much as we can a collection implementation from client code, for example, consider storing catalog objects in hibernate or some ORM. They actually use their own lazy collections of sorts, allowing client code to set them would break this scheme. Now having the Impl classes have the setters for the collecitons, that is ok. I understand this is a problem for the rest api since the xstream depersister returns back objects that have null collections. For the most part this is handled in the catalog... look at catalog.add for the various object types and you will see code that "resolves" the collections, ie, sets them by casting to the Impl class. Now again I realize this doesn't help in the rest config case because again, the object has not been added to the catalog. But i would prefer that CatalogBuilder take the same approach, and simply cast to the Impl class. Regarding the changes to CatalogBuilder, seems there are a lot of them. Can you provide a more detailed description of what was changed. Also, please ensure that only the minimum changes are there, it seems there is more in the patch than just calculating the coverage dimensions.
        Hide
        Tony Young added a comment - - edited

        Hi Justin,

        The changes to CatalogBuilder were from the original patch file so the reasons about that follow from the ones Rob originally made.

        I've removed those the new patch just initializes any null s to empty collections after casting to the Impl class.

        Show
        Tony Young added a comment - - edited Hi Justin, The changes to CatalogBuilder were from the original patch file so the reasons about that follow from the ones Rob originally made. I've removed those the new patch just initializes any null s to empty collections after casting to the Impl class.
        Hide
        Justin Deoliveira added a comment -

        Thanks Tony, this is starting to look much better I think, much easier to focus on the relevant changes when the patch is constrained.

        I would like to add Andrea to the review to get his take as he has more of an idea of possibly implications in the coverage world, but the patch looks good to me. One thing that would be nice is if it included a test case.

        Show
        Justin Deoliveira added a comment - Thanks Tony, this is starting to look much better I think, much easier to focus on the relevant changes when the patch is constrained. I would like to add Andrea to the review to get his take as he has more of an idea of possibly implications in the coverage world, but the patch looks good to me. One thing that would be nice is if it included a test case.
        Hide
        Tony Young added a comment -

        Hi Justin,

        Here's the patch with a test case for testing if the lists are correctly being made not null.

        Show
        Tony Young added a comment - Hi Justin, Here's the patch with a test case for testing if the lists are correctly being made not null.
        Hide
        Justin Deoliveira added a comment -

        Great, thanks for that Tony. As soon as we get a sanity check from Andrea I Will commit the patch. Thanks again for the contribution and for the willingness to rework it.

        Show
        Justin Deoliveira added a comment - Great, thanks for that Tony. As soon as we get a sanity check from Andrea I Will commit the patch. Thanks again for the contribution and for the willingness to rework it.
        Hide
        Andrea Aime added a comment -

        Hi,
        tried to apply the patch both on trunk and 2.1.x, but it seems it's working only partially:

         patch -p0 < /tmp/geos-4753-coverageinfo-v3.patch 
        patching file src/main/src/main/java/org/geoserver/catalog/CatalogBuilder.java
        Hunk #1 FAILED at 11.
        Hunk #2 succeeded at 848 (offset 30 lines).
        Hunk #3 succeeded at 906 (offset 30 lines).
        1 out of 3 hunks FAILED -- saving rejects to file src/main/src/main/java/org/geoserver/catalog/CatalogBuilder.java.rej
        patching file src/restconfig/src/test/java/org/geoserver/catalog/rest/CoverageTest.java
        

        As for the changes, maybe it's just the misalingment of the patch, but it seems to hit mostly the CatalogBuilder.buildCoverage
        method, which builds internally a new CoverageInfo: how can it be that the collections are null in that case?
        Why riddle the code with null checks?

        If I understand what the test code you're creating a new coverage by posting the xml to configure it, and CoverageResource.handleObjectPost()
        calls CatalogBuilder initCoverage method instead. The patch seems to involve the buildCoverage one instead

        Show
        Andrea Aime added a comment - Hi, tried to apply the patch both on trunk and 2.1.x, but it seems it's working only partially: patch -p0 < /tmp/geos-4753-coverageinfo-v3.patch patching file src/main/src/main/java/org/geoserver/catalog/CatalogBuilder.java Hunk #1 FAILED at 11. Hunk #2 succeeded at 848 (offset 30 lines). Hunk #3 succeeded at 906 (offset 30 lines). 1 out of 3 hunks FAILED -- saving rejects to file src/main/src/main/java/org/geoserver/catalog/CatalogBuilder.java.rej patching file src/restconfig/src/test/java/org/geoserver/catalog/ rest /CoverageTest.java As for the changes, maybe it's just the misalingment of the patch, but it seems to hit mostly the CatalogBuilder.buildCoverage method, which builds internally a new CoverageInfo: how can it be that the collections are null in that case? Why riddle the code with null checks? If I understand what the test code you're creating a new coverage by posting the xml to configure it, and CoverageResource.handleObjectPost() calls CatalogBuilder initCoverage method instead. The patch seems to involve the buildCoverage one instead
        Hide
        Andrea Aime added a comment -

        Removed last comment, it was meant for another Jira ticket

        Show
        Andrea Aime added a comment - Removed last comment, it was meant for another Jira ticket
        Hide
        Tony Young added a comment - - edited

        Hi Andrea,

        Should I move the null checks that I placed in buildCoverage into initCoverage, if that's a more appropriate place to put it?

        As for the null checks, it seems that sending a POST request with missing fields does not initialize things to empty lists and they end up being null and mess up all the data.

        Show
        Tony Young added a comment - - edited Hi Andrea, Should I move the null checks that I placed in buildCoverage into initCoverage , if that's a more appropriate place to put it? As for the null checks, it seems that sending a POST request with missing fields does not initialize things to empty lists and they end up being null and mess up all the data.
        Hide
        Andrea Aime added a comment -

        As far as I can see (Justin please correct me) a POST request will hit CoverageResource.handleObjectPost(), here:
        http://svn.codehaus.org/geoserver/branches/2.1.x/src/restconfig/src/main/java/org/geoserver/catalog/rest/CoverageResource.java

        The code in that method reads:

        CatalogBuilder builder = new CatalogBuilder(catalog);
        builder.setStore(coverage.getStore());
        builder.initCoverage(coverage);
        

        and there is no trace of buildCoverage there.
        So if this is the code path you're hitting (which seems to, since your CoverageInfo was initially
        parsed by XStream, something that does not happen if you just upload a new file, but it's something
        you should confirm by setting a breakpoint with a debugger) then you should work on the initCoverage
        method and not on the buildCoverage(), which, as I said, builds a new CoverageInfo during its execution,
        one that has all collections already initialized (as far as I know, at least).

        Show
        Andrea Aime added a comment - As far as I can see (Justin please correct me) a POST request will hit CoverageResource.handleObjectPost(), here: http://svn.codehaus.org/geoserver/branches/2.1.x/src/restconfig/src/main/java/org/geoserver/catalog/rest/CoverageResource.java The code in that method reads: CatalogBuilder builder = new CatalogBuilder(catalog); builder.setStore(coverage.getStore()); builder.initCoverage(coverage); and there is no trace of buildCoverage there. So if this is the code path you're hitting (which seems to, since your CoverageInfo was initially parsed by XStream, something that does not happen if you just upload a new file, but it's something you should confirm by setting a breakpoint with a debugger) then you should work on the initCoverage method and not on the buildCoverage(), which, as I said, builds a new CoverageInfo during its execution, one that has all collections already initialized (as far as I know, at least).
        Hide
        Justin Deoliveira added a comment -

        Ah, indeed Andrea is correct... indeed buildCoverage() actually doesn't get hit unless you use the file upload endpoint... a PUT or POST on a coverage resource directly won't, so yeah, looks initCoverage() is the place to work as Andrea noted.

        Show
        Justin Deoliveira added a comment - Ah, indeed Andrea is correct... indeed buildCoverage() actually doesn't get hit unless you use the file upload endpoint... a PUT or POST on a coverage resource directly won't, so yeah, looks initCoverage() is the place to work as Andrea noted.
        Hide
        Tony Young added a comment -

        Hi Justin and Andrea,

        Looks like I've misinterpreted the bug report completely (whoops!) and this patch actually addresses the issues outlined, with a test case provided.

        Show
        Tony Young added a comment - Hi Justin and Andrea, Looks like I've misinterpreted the bug report completely (whoops!) and this patch actually addresses the issues outlined, with a test case provided.
        Hide
        Justin Deoliveira added a comment -

        Hi Tony,

        No problem, thanks for the continued effort. Looking at the new patch I notice that essentially the call to CoverageInfo.setGrid() has been moved from buildCoverage() to initCoverage(). This seems like a potential regression since buildCoverage() does not call initCoverage(), so any call to buildCoverage() will now result in a null grid. So I am thinking we should just leave that line in there. Let me know if I have missed something. Other than that the patch looks good to me. Thanks!

        -Justin

        Show
        Justin Deoliveira added a comment - Hi Tony, No problem, thanks for the continued effort. Looking at the new patch I notice that essentially the call to CoverageInfo.setGrid() has been moved from buildCoverage() to initCoverage(). This seems like a potential regression since buildCoverage() does not call initCoverage(), so any call to buildCoverage() will now result in a null grid. So I am thinking we should just leave that line in there. Let me know if I have missed something. Other than that the patch looks good to me. Thanks! -Justin
        Hide
        Tony Young added a comment -

        Add setGrid to CatalogBuilder in case it is null.

        Show
        Tony Young added a comment - Add setGrid to CatalogBuilder in case it is null .
        Hide
        Tony Young added a comment -

        Add setGrid to CatalogBuilder in case it is null.

        Show
        Tony Young added a comment - Add setGrid to CatalogBuilder in case it is null .
        Hide
        Justin Deoliveira added a comment -

        Patch applied. Thanks again!

        Show
        Justin Deoliveira added a comment - Patch applied. Thanks again!
        Hide
        Andrea Aime added a comment -

        Mass transition all resolved issue that did not see any further comment in the last month to closed status

        Show
        Andrea Aime added a comment - Mass transition all resolved issue that did not see any further comment in the last month to closed status

          People

          • Assignee:
            Justin Deoliveira
            Reporter:
            Robert Coup
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: