[GEOS-11595] CoverageRATs check for PAM Resource is failing when checking CoverageViews #8025

turingtestfail · 2024-11-14T18:30:19Z

pieces in place

fixed indexing issue

cleanup

added test

Checklist

I have read the contribution guidelines.
I have sent a Contribution Licence Agreement (not required for small changes, e.g., fixing typos in documentation).
First PR targets the main branch (backports managed later; ignore for branch specific issues).
All the build checks are green (see automated QA checks).

For core and extension modules:

New unit tests have been added covering the changes.
Documentation has been updated (if change is visible to end users).
The REST API docs have been updated (when changing configuration objects or the REST controllers).
There is an issue in the GeoServer Jira (except for changes that do not affect administrators or end users in any way).
Commit message(s) must be in the form [GEOS-XYZWV] Title of the Jira ticket.
Bug fixes and small new features are presented as a single commit.
Each commit has a single objective (if there are multiple commits, each has a separate JIRA ticket describing its goal).

aaime

Looks overall good, see comments below

aaime · 2024-12-03T10:06:16Z

src/main/src/main/java/org/geoserver/catalog/CoverageViewReader.java

+        return null;
+    }
+
+    private ResourceInfo createResourceInfoFromView() {


Basically a one liner, can be in-lined

aaime · 2024-12-03T10:06:22Z

src/main/src/main/java/org/geoserver/catalog/CoverageViewReader.java

+        return resourceInfo;
+    }
+
+    private ResourceInfo createPAMResourceInfoFromView(


Same as above

aaime · 2024-12-03T10:07:07Z

src/main/src/main/java/org/geoserver/catalog/CoverageViewResourceInfo.java

+/** A ResourceInfo wrapper for CoverageViews */
+public class CoverageViewResourceInfo extends DefaultResourceInfo {
+
+    public CoverageViewResourceInfo(CoverageView coverageView) {}


No need for this subclass, just return DefaultResourceInfo, it can be instantiated

aaime · 2024-12-03T10:13:55Z

src/main/src/main/java/org/geoserver/catalog/CoverageViewReader.java

+        return inputAlphaNonNull;
+    }
+
+    public static class InputAlphaNonNullCoverages {


Needs a better name... maybe ViewInputs? Also, does not seem like it needs to be public, package private should be enough.

Changed and made package private.

aaime · 2024-12-03T10:14:32Z

src/main/src/main/java/org/geoserver/catalog/CoverageViewPamResourceInfo.java

+        for (CoverageView.CoverageBand band : info.getBands()) {
+            for (CoverageView.InputCoverageBand inputCoverageBand : band.getInputCoverageBands()) {
+                // get the reader for the coverage associated with this input coverage band
+                GridCoverageReader reader1 = readers.get(inputCoverageBand.getCoverageName());


Don't see the need for the "1" suffix.

Suggested change

GridCoverageReader reader1 = readers.get(inputCoverageBand.getCoverageName());

GridCoverageReader reader = readers.get(inputCoverageBand.getCoverageName());

aaime · 2024-12-03T10:16:06Z

src/main/src/main/java/org/geoserver/catalog/CoverageViewPamResourceInfo.java

+     *
+     * @param inputAlphaNonNullCoverages input alpha non-null coverages
+     */
+    public CoverageViewPamResourceInfo(


Does not need like you need the coverages to build the "info" object, so I'd avoid collecting them, reading the raster can allocate actual memory for nothing, depending on the data source (e.g., GeoTIFF does delayed loading, but NetCDF does not).

coverages instantiation made optional

aaime · 2024-12-03T10:18:03Z

src/extension/rat/src/test/java/org/geoserver/rat/RATViewTest.java

@@ -0,0 +1,126 @@
+package org.geoserver.rat;


Copyright header missing

…king CoverageViews pieces in place fixed indexing issue cleanup added test transitioning to creating ResourceInfo more cleanup cleanup cleanup pmd missing copyright cleanup style PR Changes

turingtestfail requested a review from aaime November 15, 2024 16:54

turingtestfail force-pushed the GEOS-11595-CoverageRats-PAM-CoverageViews branch 4 times, most recently from 3d8470b to 7f7b328 Compare November 27, 2024 17:20

aaime requested changes Dec 3, 2024

View reviewed changes

[GEOS-11595] CoverageRATs check for PAM Resource is failing when chec…

6a66bbf

…king CoverageViews pieces in place fixed indexing issue cleanup added test transitioning to creating ResourceInfo more cleanup cleanup cleanup pmd missing copyright cleanup style PR Changes

turingtestfail force-pushed the GEOS-11595-CoverageRats-PAM-CoverageViews branch from 7f7b328 to 6a66bbf Compare December 3, 2024 18:14

aaime approved these changes Dec 5, 2024

View reviewed changes

aaime merged commit e9b06e1 into geoserver:main Dec 5, 2024
16 checks passed

aaime added backport 2.25.x Instructs the bot to create a 2.25.x backport PR on merge backport 2.26.x Instructs the bot to create a 2.26.x backport PR on merge labels Dec 5, 2024

This was referenced Dec 5, 2024

[Backport 2.26.x] [GEOS-11595] CoverageRATs check for PAM Resource is failing when checking CoverageViews #8085

Merged

[Backport 2.25.x] [GEOS-11595] CoverageRATs check for PAM Resource is failing when checking CoverageViews #8086

Merged

	GridCoverageReader reader1 = readers.get(inputCoverageBand.getCoverageName());
	GridCoverageReader reader = readers.get(inputCoverageBand.getCoverageName());

Uh oh!

[GEOS-11595] CoverageRATs check for PAM Resource is failing when checking CoverageViews #8025

[GEOS-11595] CoverageRATs check for PAM Resource is failing when checking CoverageViews #8025

Uh oh!

Conversation

turingtestfail commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

aaime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

turingtestfail commented Nov 14, 2024 •

edited

Loading