Skip to content

Commit

Permalink
Fix saving workspace services not working with pgconfig
Browse files Browse the repository at this point in the history
Saving the workspace services not working with pgconfig when
caching is enabled. `CachingGeoServerFacade` assumed there's
nothing to do when a `ServiceInfo` is added for a workspace,
but it was caching the `null` value, which is expected to save
db roundtrips.

- Add missing treatment of `ServiceInfoAdded` and `SettingInfoAdded`
to `CachingGeoServerFacade`.

- Add referential integrity for settingsinfo and serviceinfo

Now the flyway migration will add referential integrity for:

 - `settingsinfo(workspace) -> workspaceindo(id)`
 - `serviceinfo(workspace) -> workspaceinfo(id)`

And unique indexes for:

- `settingsinfo(workspace)`
- `serviceinfo("@type", workspace)`

As a side effect of the caching facade returning null once a
`ServiceInfo` as added, you could add multiple services of the
same kind for a workspace. The referential integrity checks will
avoid that, and the FlyWay migration will delete existing duplicates.
  • Loading branch information
groldan committed Oct 26, 2024
1 parent a293129 commit 33dfdc5
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 8 deletions.
2 changes: 2 additions & 0 deletions compose/.env
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ TAG=1.9-SNAPSHOT
ACL_REPOSITORY=geoservercloud
ACL_TAG=2.3.1

GATEWAY_PORT=9090

GS_USER="1000:1000"
GATEWAY_SHARED_AUTH=true

Expand Down
2 changes: 1 addition & 1 deletion compose/compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ services:
SPRING_PROFILES_ACTIVE: "${GATEWAY_DEFAULT_PROFILES}"
GATEWAY_SHARED_AUTH: "${GATEWAY_SHARED_AUTH}" #same as in gstemplate
ports:
- 9090:8080
- ${GATEWAY_PORT}:8080
deploy:
resources:
limits:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Add referential integrity constraint from:
*
* - settingsinfo(workspace) -> workspaceindo(id)
* - serviceinfo(workspace) -> workspaceinfo(id)
*
* And unique indexes for:
*
* - settingsinfo(workspace)
* - serviceinfo("@type", workspace)
*/


ALTER TABLE settingsinfo
ADD CONSTRAINT fk_settingsinfo_workspace
FOREIGN KEY (workspace)
REFERENCES workspaceinfo(id)
ON DELETE CASCADE;

CREATE UNIQUE INDEX settingsinfo_workspace_key
ON settingsinfo (workspace)
NULLS NOT DISTINCT;

/*
* Delete possible duplicate serviceinfos by type and workspace before enforcing uniqueness.
*/
DELETE FROM serviceinfo WHERE id IN( select id FROM (
SELECT id,
ROW_NUMBER() OVER(PARTITION BY "@type", workspace ORDER BY id ASC) AS duprow
FROM serviceinfo
) dups
WHERE
dups.duprow > 1);

ALTER TABLE serviceinfo
ADD CONSTRAINT fk_serviceinfo_workspace
FOREIGN KEY (workspace)
REFERENCES workspaceinfo(id)
ON DELETE CASCADE;

CREATE UNIQUE INDEX serviceinfo_workspace_key
ON serviceinfo ("@type", workspace)
NULLS NOT DISTINCT;
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,26 @@
*/
package org.geoserver.cloud.backend.pgconfig.config;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;

import org.geoserver.catalog.Catalog;
import org.geoserver.catalog.WorkspaceInfo;
import org.geoserver.cloud.backend.pgconfig.PgconfigBackendBuilder;
import org.geoserver.cloud.backend.pgconfig.support.PgConfigTestContainer;
import org.geoserver.config.GeoServer;
import org.geoserver.config.GeoServerConfigConformanceTest;
import org.geoserver.config.ServiceInfo;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.testcontainers.junit.jupiter.Container;
import org.testcontainers.junit.jupiter.Testcontainers;

/**
* @since 1.4
*/
@Testcontainers(disabledWithoutDocker = true)
@SuppressWarnings("java:S2187")
class PgconfigConfigRepositoryConformanceTest extends GeoServerConfigConformanceTest {

@Container static PgConfigTestContainer<?> container = new PgConfigTestContainer<>();
Expand All @@ -40,4 +45,32 @@ void cleanDb() {
Catalog catalog = builder.createCatalog();
return builder.createGeoServer(catalog);
}

@Test
void testAddDuplicateServiceToWorkspace() {
// Make a workspace
WorkspaceInfo ws1 = geoServer.getCatalog().getFactory().createWorkspace();
ws1.setName("TEST-WORKSPACE-1");
geoServer.getCatalog().add(ws1);

// Make a service for that workspace
ServiceInfo service1 = createService();
service1.setWorkspace(ws1);
service1.setName("TEST-OWS");
service1.setTitle("Service for WS1");
geoServer.add(service1);

ServiceInfo dupTypeAndWorkspace = createService();
dupTypeAndWorkspace.setWorkspace(ws1);
dupTypeAndWorkspace.setName("TEST-OWS");
dupTypeAndWorkspace.setTitle("Service for WS1");

var ex =
assertThrows(
IllegalArgumentException.class, () -> geoServer.add(dupTypeAndWorkspace));

assertThat(ex.getMessage())
.contains(
"service with name 'TEST-OWS' already exists in workspace 'TEST-WORKSPACE-1'");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
import org.geoserver.cloud.event.config.GeoServerInfoSet;
import org.geoserver.cloud.event.config.LoggingInfoModified;
import org.geoserver.cloud.event.config.LoggingInfoSet;
import org.geoserver.cloud.event.config.ServiceAdded;
import org.geoserver.cloud.event.config.ServiceModified;
import org.geoserver.cloud.event.config.ServiceRemoved;
import org.geoserver.cloud.event.config.SettingsAdded;
import org.geoserver.cloud.event.config.SettingsModified;
import org.geoserver.cloud.event.config.SettingsRemoved;
import org.geoserver.cloud.event.info.ConfigInfoType;
Expand Down Expand Up @@ -90,8 +92,8 @@ void onLoggingInfoModifyEvent(LoggingInfoModified event) {
evictConfigEntry(event);
}

@EventListener(classes = SettingsModified.class)
void onSettingsInfoModifyEvent(SettingsModified event) {
@EventListener(classes = ServiceAdded.class)
void onServiceInfoAddEvent(ServiceAdded event) {
evictConfigEntry(event);
}

Expand All @@ -100,13 +102,23 @@ void onServiceInfoModifyEvent(ServiceModified event) {
evictConfigEntry(event);
}

@EventListener(classes = SettingsRemoved.class)
void onSettingsInfoRemoveEvent(SettingsRemoved event) {
@EventListener(classes = ServiceRemoved.class)
void onServiceInfoRemoveEvent(ServiceRemoved event) {
evictConfigEntry(event);
}

@EventListener(classes = ServiceRemoved.class)
void onServiceInfoRemoveEvent(ServiceRemoved event) {
@EventListener(classes = SettingsAdded.class)
void onSettingsInfoAddEvent(SettingsAdded event) {
evictConfigEntry(event);
}

@EventListener(classes = SettingsModified.class)
void onSettingsInfoModifyEvent(SettingsModified event) {
evictConfigEntry(event);
}

@EventListener(classes = SettingsRemoved.class)
void onSettingsInfoRemoveEvent(SettingsRemoved event) {
evictConfigEntry(event);
}

Expand Down

0 comments on commit 33dfdc5

Please sign in to comment.