RESTConfig doesn't calculate Coverage grid/dimensions

Patches are against the 2.1.x branch

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.

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?

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.

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.

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.

Hi Justin,

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

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.

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

Removed last comment, it was meant for another Jira ticket

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.

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).

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.

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.

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

Add setGrid to CatalogBuilder in case it is null.

Add setGrid to CatalogBuilder in case it is null.

Patch applied. Thanks again!