GeoServer

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.

Attachments

  1. Text File
    geos-4753-coverageinfo.patch
    21/Nov/11 8:34 PM
    27 kB
    Tony Young
  2. Text File
    geos-4753-coverageinfo-v2.patch
    23/Nov/11 6:00 PM
    9 kB
    Tony Young
  3. Text File
    geos-4753-coverageinfo-v3.patch
    24/Nov/11 7:26 PM
    11 kB
    Tony Young
  4. Text File
    geos-4753-coverageinfo-v3.patch
    24/Nov/11 4:09 PM
    11 kB
    Tony Young
  5. Text File
    geos-4753-coverageinfo-v4.patch
    30/Nov/11 6:19 PM
    3 kB
    Tony Young
  6. Text File
    geos-4753-coverageinfo-v5.patch
    01/Dec/11 9:04 PM
    2 kB
    Tony Young
  7. Text File
    geos-4753-coverageinfo-v5.patch
    01/Dec/11 9:04 PM
    2 kB
    Tony Young
  8. Text File
    rest_coverageinfo.contentOnly.patch
    06/Sep/11 1:05 AM
    24 kB
    Robert Coup
  9. Text File
    rest_coverageinfo.patch
    06/Sep/11 1:05 AM
    29 kB
    Robert Coup

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Justin Deoliveira added a comment -

Patch applied. Thanks again!

Show
Justin Deoliveira added a comment - Patch applied. Thanks again!
Hide
Permalink
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

Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.