diff --git a/.github/actions/setup-r/action.yml b/.github/actions/setup-r/action.yml index 127588d238..5976bc91a9 100644 --- a/.github/actions/setup-r/action.yml +++ b/.github/actions/setup-r/action.yml @@ -40,7 +40,22 @@ runs: - name: Set up TinyTeX uses: r-lib/actions/setup-tinytex@v2 - # libcurl is required for building the R package documentation. - - name: Install libcurl + # Native libraries required to build the R package and its documentation + # toolchain from source. When the R package cache misses, devtools and + # pkgdown are compiled from source, which pulls in systemfonts, textshaping + # and ragg. Those packages need the font (fontconfig, freetype, harfbuzz, + # fribidi) and image (png, tiff, jpeg) development headers, and libcurl is + # needed for the documentation build. + - name: Install system libraries shell: bash - run: sudo apt-get install -y libcurl4-openssl-dev + run: | + sudo apt-get update + sudo apt-get install -y \ + libcurl4-openssl-dev \ + libfontconfig1-dev \ + libfreetype-dev \ + libharfbuzz-dev \ + libfribidi-dev \ + libpng-dev \ + libtiff-dev \ + libjpeg-dev diff --git a/.github/workflows/server-pre-release.yml b/.github/workflows/server-pre-release.yml index 0ac0fa97f5..1772191e2d 100644 --- a/.github/workflows/server-pre-release.yml +++ b/.github/workflows/server-pre-release.yml @@ -17,7 +17,7 @@ jobs: pre-release-server: name: Build and push Docker image runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 105 steps: - name: Checkout code uses: actions/checkout@v4 @@ -68,12 +68,12 @@ jobs: env: SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} run: | - mvn --batch-mode deploy -PdockerPreRelease -Dmaven.deploy.skip \ + mvn --batch-mode --update-snapshots deploy -PdockerPreRelease -Dmaven.deploy.skip \ org.sonarsource.scanner.maven:sonar-maven-plugin:sonar \ -Dsonar.projectKey=aehrc_pathling_server -Dsonar.organization=aehrc \ -Dsonar.host.url=https://sonarcloud.io \ -Dsonar.sarifReportPaths=../trivy-results-server.sarif - timeout-minutes: 30 + timeout-minutes: 90 - name: Save test reports if: always() diff --git a/.github/workflows/server-release.yml b/.github/workflows/server-release.yml index 209fa71e23..c12da19a36 100644 --- a/.github/workflows/server-release.yml +++ b/.github/workflows/server-release.yml @@ -17,7 +17,7 @@ jobs: name: Build and push Docker image if: github.event_name == 'workflow_dispatch' || startsWith(github.ref_name, 'server-v') runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 105 steps: - name: Checkout code uses: actions/checkout@v4 @@ -65,7 +65,7 @@ jobs: -Dsonar.projectKey=aehrc_pathling_server -Dsonar.organization=aehrc \ -Dsonar.host.url=https://sonarcloud.io \ -Dsonar.sarifReportPaths=../trivy-results-server.sarif - timeout-minutes: 30 + timeout-minutes: 90 - name: Save test reports if: always() diff --git a/.github/workflows/server-test.yml b/.github/workflows/server-test.yml index 5ecc9ca929..0127052edd 100644 --- a/.github/workflows/server-test.yml +++ b/.github/workflows/server-test.yml @@ -22,7 +22,7 @@ jobs: test-server: name: Build and test server runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 45 steps: - name: Checkout code uses: actions/checkout@v4 @@ -88,7 +88,7 @@ jobs: -Dsonar.projectKey=aehrc_pathling_server -Dsonar.organization=aehrc \ -Dsonar.host.url=https://sonarcloud.io \ -Dsonar.sarifReportPaths=trivy-results.sarif - timeout-minutes: 30 + timeout-minutes: 45 - name: Save test reports if: always() diff --git a/.github/workflows/ui-test.yml b/.github/workflows/ui-test.yml index 8a49e3b46c..a2daaab640 100644 --- a/.github/workflows/ui-test.yml +++ b/.github/workflows/ui-test.yml @@ -1,3 +1,8 @@ +# Runs the Admin UI quality checks documented in ui/CONTRIBUTING.md. Called by +# both test.yml and pre-release.yml. +# +# Author: John Grimes + name: Test UI on: @@ -27,11 +32,14 @@ jobs: - name: Lint run: bun run lint + - name: Check duplication + run: bun run lint:duplication + - name: Build run: bun run build - - name: Run unit tests - run: bun run test:run + - name: Run unit tests with coverage + run: bun run test:coverage - name: Install Playwright browsers # The pinned version must be kept in step with @playwright/test in diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 330dbebfd2..d4d9fb813f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -150,16 +150,34 @@ mvn clean install -pl lib/R -am ### Clearing the Ivy cache -When rebuilding after making changes to upstream modules, you may need to clear -the local Ivy cache before the changes will be picked up by the Python and R -libraries. The Ivy cache is typically located at `~/.ivy2/cache`. - -To clear the cache: +When rebuilding after making changes to upstream modules, you need to clear the +local Ivy state before the changes will be picked up by the Python and R +libraries. + +Spark resolves `--packages` through Ivy, which keeps two directories per Ivy +version: `cache` (the resolved module metadata and artifacts) and `jars` (the +jars retrieved for the run). Both must be cleared. Removing only `cache` leaves +`au.csiro.pathling_library-runtime--SNAPSHOT.jar` in `jars`, and +because the SNAPSHOT filename never changes, Ivy reports `0 artifacts copied, N +already retrieved` and silently reuses the stale build. The tests still run and +pass; they just do not exercise the new code. + +Spark also uses a home directory suffixed with its bundled Ivy version (for +example `~/.ivy2.5.2`) in addition to the conventional `~/.ivy2`, so both trees +need to be cleared: ```bash -rm -rf ~/.ivy2/cache +rm -rf ~/.ivy2/cache ~/.ivy2/jars ~/.ivy2.5.2/cache ~/.ivy2.5.2/jars +mkdir -p ~/.ivy2/cache ~/.ivy2/jars ~/.ivy2.5.2/cache ~/.ivy2.5.2/jars ``` +Run `ls -d ~/.ivy2*` to confirm which version-suffixed directories exist on your +machine, as the suffix follows the Spark version in use. + +The `mkdir` step matters. With the `jars` directory missing, Ivy fails to +retrieve `delta-spark`, `delta-storage` and `antlr4-runtime` from the local +Maven resolver, and the run fails with `JAVA_GATEWAY_EXITED`. + After clearing the cache, rebuild the libraries: ```bash diff --git a/benchmark/pom.xml b/benchmark/pom.xml index db7c94f2f0..366ed100d9 100644 --- a/benchmark/pom.xml +++ b/benchmark/pom.xml @@ -23,7 +23,7 @@ au.csiro.pathling pathling - 9.8.0 + 9.9.0-SNAPSHOT benchmark jar diff --git a/deployment/cache/README.md b/deployment/cache/README.md index 7d4147b8c1..acf685537c 100644 --- a/deployment/cache/README.md +++ b/deployment/cache/README.md @@ -8,6 +8,10 @@ based upon the deployment: - `PATHLING_HOST`: The host name of the Pathling server. - `PATHLING_PORT`: The port number exposed by the Pathling server. +- `PATHLING_FIRST_BYTE_TIMEOUT`: The maximum time to wait for the first byte of + a backend response, expressed as a Varnish duration (e.g. `60s`, `10m`). Raise + this above the Varnish default of `60s` to accommodate long-running + synchronous Pathling queries. -Copyright © 2065, Commonwealth Scientific and Industrial Research Organisation +Copyright © 2025, Commonwealth Scientific and Industrial Research Organisation (CSIRO) ABN 41 687 119 230. Licensed under the Apache License, Version 2.0. diff --git a/deployment/cache/chart/Chart.yaml b/deployment/cache/chart/Chart.yaml index b03d0ce35b..2f6a27d371 100644 --- a/deployment/cache/chart/Chart.yaml +++ b/deployment/cache/chart/Chart.yaml @@ -8,7 +8,7 @@ name: pathling-cache description: A Varnish-based frontend cache optimised for use with Pathling Server icon: https://raw.githubusercontent.com/aehrc/pathling/main/media/logo-icon-colour-detail.svg type: application -version: 1.0.0 +version: 1.1.0 maintainers: - name: John Grimes email: John.Grimes@csiro.au diff --git a/deployment/cache/chart/README.md b/deployment/cache/chart/README.md index beb6191842..2a42a798a0 100644 --- a/deployment/cache/chart/README.md +++ b/deployment/cache/chart/README.md @@ -26,19 +26,20 @@ helm install my-cache ./chart \ ## Configuration -| Parameter | Description | Default | -| ------------------------------- | ------------------------------------------ | ------------------------------------- | -| `pathlingCache.image` | Container image to use | `ghcr.io/aehrc/pathling-cache:latest` | -| `pathlingCache.imagePullPolicy` | Image pull policy | `IfNotPresent` | -| `pathlingCache.replicas` | Number of replicas to deploy | `1` | -| `pathlingCache.pathlingHost` | Hostname of the Pathling server (required) | `~` | -| `pathlingCache.pathlingPort` | Port of the Pathling server (required) | `~` | -| `pathlingCache.service.type` | Kubernetes service type | `ClusterIP` | -| `pathlingCache.service.port` | Service port | `80` | -| `pathlingCache.resources` | CPU/memory resource requests and limits | `{}` | -| `pathlingCache.tolerations` | Pod tolerations | `[]` | -| `pathlingCache.affinity` | Pod affinity rules | `{}` | -| `pathlingCache.nodeSelector` | Node selector labels | `{}` | +| Parameter | Description | Default | +| -------------------------------- | --------------------------------------------------------- | ------------------------------------- | +| `pathlingCache.image` | Container image to use | `ghcr.io/aehrc/pathling-cache:latest` | +| `pathlingCache.imagePullPolicy` | Image pull policy | `IfNotPresent` | +| `pathlingCache.replicas` | Number of replicas to deploy | `1` | +| `pathlingCache.pathlingHost` | Hostname of the Pathling server (required) | `~` | +| `pathlingCache.pathlingPort` | Port of the Pathling server (required) | `~` | +| `pathlingCache.firstByteTimeout` | Backend first-byte timeout (Varnish duration, e.g. `10m`) | `60s` | +| `pathlingCache.service.type` | Kubernetes service type | `ClusterIP` | +| `pathlingCache.service.port` | Service port | `80` | +| `pathlingCache.resources` | CPU/memory resource requests and limits | `{}` | +| `pathlingCache.tolerations` | Pod tolerations | `[]` | +| `pathlingCache.affinity` | Pod affinity rules | `{}` | +| `pathlingCache.nodeSelector` | Node selector labels | `{}` | ## Examples diff --git a/deployment/cache/chart/templates/deployment.yaml b/deployment/cache/chart/templates/deployment.yaml index ed6afd4c50..79393aa2d7 100644 --- a/deployment/cache/chart/templates/deployment.yaml +++ b/deployment/cache/chart/templates/deployment.yaml @@ -32,6 +32,8 @@ spec: value: {{ required "pathlingCache.pathlingHost is required" .Values.pathlingCache.pathlingHost | quote }} - name: PATHLING_PORT value: {{ required "pathlingCache.pathlingPort is required" .Values.pathlingCache.pathlingPort | quote }} + - name: PATHLING_FIRST_BYTE_TIMEOUT + value: {{ .Values.pathlingCache.firstByteTimeout | quote }} livenessProbe: tcpSocket: port: http diff --git a/deployment/cache/chart/values.schema.json b/deployment/cache/chart/values.schema.json index 17ae6f7a34..75a5970282 100644 --- a/deployment/cache/chart/values.schema.json +++ b/deployment/cache/chart/values.schema.json @@ -33,6 +33,11 @@ "type": ["integer", "null"], "description": "Port of the Pathling server to cache (required)" }, + "firstByteTimeout": { + "type": "string", + "description": "Maximum time the cache waits for the first byte of a backend response, as a Varnish duration (e.g. '60s', '10m')", + "default": "60s" + }, "service": { "type": "object", "description": "Kubernetes service configuration", diff --git a/deployment/cache/chart/values.yaml b/deployment/cache/chart/values.yaml index 7681d8d0cb..55ae38ab1b 100644 --- a/deployment/cache/chart/values.yaml +++ b/deployment/cache/chart/values.yaml @@ -12,6 +12,11 @@ pathlingCache: pathlingHost: ~ pathlingPort: ~ + # Maximum time the cache waits for the first byte of a backend response, + # expressed as a Varnish duration (e.g. "60s", "10m"). Raise this when + # synchronous Pathling queries take longer than the Varnish default of 60s. + firstByteTimeout: "60s" + service: type: "ClusterIP" port: 80 diff --git a/deployment/cache/default.vcl b/deployment/cache/default.vcl index efe9e4380b..f2774de876 100644 --- a/deployment/cache/default.vcl +++ b/deployment/cache/default.vcl @@ -7,9 +7,30 @@ import std; backend default { .host = "${PATHLING_HOST}"; .port = "${PATHLING_PORT}"; + // Maximum time to wait for the first byte of the backend response. Raised + // above the Varnish default of 60s to accommodate long-running synchronous + // Pathling queries. + .first_byte_timeout = ${PATHLING_FIRST_BYTE_TIMEOUT}; } sub vcl_recv { + // Bulk-data file downloads ($result) stream large NDJSON bodies to streaming + // clients such as the $import-pnp downloader. The frontend cache must not + // gzip, buffer, or cache these: on a cache miss Varnish otherwise serves an + // on-the-fly-gzipped, chunked, streamed response that is closed mid-body when + // the client reads in bursts (paced by its own downstream writes), truncating + // the download. Bypass the cache entirely so the response streams straight + // through with the backend's Content-Length. + if (req.url ~ "/\$result") { + // These clients read in bursts, stalling for as long as it takes to write + // each block to their own storage, which can exceed the default one minute + // allowed for sending to a client that is not reading. Without this, the + // connection is closed part-way through a large file and the download fails + // with a premature end of message body. + set sess.idle_send_timeout = 30m; + set sess.send_timeout = 2h; + return (pass); + } if (req.http.Cache-Control ~ "(private|no-cache|no-store)" || req.http.Pragma == "no-cache") { return (pass); } @@ -20,6 +41,16 @@ sub vcl_req_authorization { } sub vcl_backend_response { + // Never cache or gzip bulk-data file downloads (see vcl_recv). Disabling gzip + // lets the backend Content-Length pass through, so the body is delivered with + // a fixed length rather than chunked encoding, which is robust to the bursty, + // idle-gap read pattern of streaming download clients. + if (bereq.url ~ "/\$result") { + set beresp.uncacheable = true; + set beresp.do_gzip = false; + return (deliver); + } + // Respect backend cache-control headers that indicate the response should not be cached. if (beresp.http.Cache-Control ~ "(no-cache|no-store|private)") { set beresp.uncacheable = true; diff --git a/deployment/helm/pathling/templates/deployment.yaml b/deployment/helm/pathling/templates/deployment.yaml index 451ef172f8..3069a8a91b 100644 --- a/deployment/helm/pathling/templates/deployment.yaml +++ b/deployment/helm/pathling/templates/deployment.yaml @@ -55,6 +55,14 @@ spec: value: "7078" - name: spark.driver.bindAddress value: 0.0.0.0 + # In client mode, Spark only sets an ownerReference on executor + # pods when it knows the driver pod's name. This allows Kubernetes + # garbage collection to remove executor pods (and their on-demand + # PVCs) when the driver pod is deleted, e.g. on rollout restart. + - name: spark.kubernetes.driver.pod.name + valueFrom: + fieldRef: + fieldPath: metadata.name {{- range $configKey, $configValue := .Values.pathling.config }} - name: {{ $configKey }} value: {{ $configValue | quote }} diff --git a/deployment/helm/pathling/templates/role.yaml b/deployment/helm/pathling/templates/role.yaml index e5072dc652..7f29a73ed3 100644 --- a/deployment/helm/pathling/templates/role.yaml +++ b/deployment/helm/pathling/templates/role.yaml @@ -6,6 +6,11 @@ rules: - apiGroups: [""] resources: ["pods", "services", "configmaps"] verbs: ["create", "get", "list", "watch", "delete"] + # Allow the driver to provision and remove the dynamically created scratch + # volumes used for executor local storage (claimName OnDemand). + - apiGroups: [""] + resources: ["persistentvolumeclaims"] + verbs: ["create", "get", "list", "watch", "delete"] - apiGroups: [""] resources: ["pods/log"] verbs: ["get", "list", "watch"] diff --git a/encoders/pom.xml b/encoders/pom.xml index b42d2393af..e7673256dc 100644 --- a/encoders/pom.xml +++ b/encoders/pom.xml @@ -32,7 +32,7 @@ au.csiro.pathling pathling - 9.8.0 + 9.9.0-SNAPSHOT encoders jar diff --git a/encoders/src/main/java/au/csiro/pathling/encoders/ViewDefinitionResource.java b/encoders/src/main/java/au/csiro/pathling/encoders/ViewDefinitionResource.java index a424e60b4d..778b9e6dec 100644 --- a/encoders/src/main/java/au/csiro/pathling/encoders/ViewDefinitionResource.java +++ b/encoders/src/main/java/au/csiro/pathling/encoders/ViewDefinitionResource.java @@ -73,6 +73,14 @@ public class ViewDefinitionResource extends DomainResource { @Serial private static final long serialVersionUID = 1909997123685548098L; + @Nullable + @Child(name = "url") + private UriType url; + + @Nullable + @Child(name = "version") + private StringType version; + @Nullable @Getter @Child(name = "name") @@ -100,6 +108,50 @@ public class ViewDefinitionResource extends DomainResource { @Child(name = "constant", max = Child.MAX_UNLIMITED) private List constant; + @Nullable + public String getUrl() { + return url == null ? null : url.getValue(); + } + + @Nullable + public UriType getUrlElement() { + return url; + } + + public boolean hasUrlElement() { + return url != null && !url.isEmpty(); + } + + public void setUrlElement(final UriType url) { + this.url = url; + } + + public void setUrl(final String url) { + this.url = url == null ? null : new UriType(url); + } + + @Nullable + public String getVersion() { + return version == null ? null : version.getValue(); + } + + @Nullable + public StringType getVersionElement() { + return version; + } + + public boolean hasVersionElement() { + return version != null && !version.isEmpty(); + } + + public void setVersionElement(final StringType version) { + this.version = version; + } + + public void setVersion(final String version) { + this.version = version == null ? null : new StringType(version); + } + @Nullable public StringType getNameElement() { return name; @@ -187,6 +239,8 @@ public boolean hasConstant() { public DomainResource copy() { final ViewDefinitionResource copy = new ViewDefinitionResource(); copyValues(copy); + copy.url = url != null ? url.copy() : null; + copy.version = version != null ? version.copy() : null; copy.name = name != null ? name.copy() : null; if (fhirVersion != null) { copy.fhirVersion = new ArrayList<>(); @@ -233,6 +287,8 @@ public String fhirType() { @Override public boolean isEmpty() { return super.isEmpty() + && (url == null || url.isEmpty()) + && (version == null || version.isEmpty()) && (name == null || name.isEmpty()) && (fhirVersion == null || fhirVersion.isEmpty()) && (resource == null || resource.isEmpty()) diff --git a/encoders/src/test/java/au/csiro/pathling/encoders/ViewDefinitionEncodingTest.java b/encoders/src/test/java/au/csiro/pathling/encoders/ViewDefinitionEncodingTest.java index d5c200f885..9757159ce5 100644 --- a/encoders/src/test/java/au/csiro/pathling/encoders/ViewDefinitionEncodingTest.java +++ b/encoders/src/test/java/au/csiro/pathling/encoders/ViewDefinitionEncodingTest.java @@ -55,6 +55,7 @@ import org.hl7.fhir.r4.model.BooleanType; import org.hl7.fhir.r4.model.CodeType; import org.hl7.fhir.r4.model.IntegerType; +import org.hl7.fhir.r4.model.UriType; import org.json.JSONException; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; @@ -127,6 +128,8 @@ void testSchemaHasBasicFields() { // Verify top-level fields exist. assertTrue(schema.getFieldIndex("id").isDefined()); + assertTrue(schema.getFieldIndex("url").isDefined()); + assertTrue(schema.getFieldIndex("version").isDefined()); assertTrue(schema.getFieldIndex("name").isDefined()); assertTrue(schema.getFieldIndex("resource").isDefined()); assertTrue(schema.getFieldIndex("status").isDefined()); @@ -135,6 +138,49 @@ void testSchemaHasBasicFields() { assertTrue(schema.getFieldIndex("constant").isDefined()); } + @Test + void testUrlAndVersionSurviveRoundTrip() { + // The url and version are required to match dependency references by canonical URL, so they + // must be retained through an encode/decode round-trip. + final ViewDefinitionResource original = createSimpleViewDefinition(); + original.setUrlElement(new UriType("https://example.org/ViewDefinition/patients")); + original.setVersionElement(new org.hl7.fhir.r4.model.StringType("2.0")); + + final ExpressionEncoder encoder = + fhirEncodersL0.of(ViewDefinitionResource.class); + final ExpressionEncoder resolvedEncoder = + EncoderUtils.defaultResolveAndBind(encoder); + + final InternalRow serializedRow = resolvedEncoder.createSerializer().apply(original); + final ViewDefinitionResource decoded = + resolvedEncoder.createDeserializer().apply(serializedRow); + + assertTrue(original.equalsDeep(decoded)); + assertEquals("https://example.org/ViewDefinition/patients", decoded.getUrl()); + assertEquals("2.0", decoded.getVersion()); + } + + @Test + void testDecodeWithoutUrlAndVersionYieldsEmptyValues() { + // A ViewDefinition stored without a url or version (the pre-existing case) must decode cleanly + // with absent url and version, leaving it unmatchable by canonical URL. + final ViewDefinitionResource original = createSimpleViewDefinition(); + + final ExpressionEncoder encoder = + fhirEncodersL0.of(ViewDefinitionResource.class); + final ExpressionEncoder resolvedEncoder = + EncoderUtils.defaultResolveAndBind(encoder); + + final InternalRow serializedRow = resolvedEncoder.createSerializer().apply(original); + final ViewDefinitionResource decoded = + resolvedEncoder.createDeserializer().apply(serializedRow); + + assertFalse(decoded.hasUrlElement()); + assertFalse(decoded.hasVersionElement()); + assertNull(decoded.getUrl()); + assertNull(decoded.getVersion()); + } + @Test void testSchemaHandlesRecursiveSelectComponent() { // Level 0: should NOT have nested select.select or unionAll. @@ -846,4 +892,84 @@ void testFhirType() { final ViewDefinitionResource view = new ViewDefinitionResource(); assertEquals("ViewDefinition", view.fhirType()); } + + // ========== URL AND VERSION ACCESSOR TESTS ========== + + @Test + void testSetUrlWithStringValue() { + // setUrl(String) wraps the value in a UriType and getUrl() unwraps it. + final ViewDefinitionResource view = new ViewDefinitionResource(); + view.setUrl("https://example.org/ViewDefinition/patients"); + + assertTrue(view.hasUrlElement()); + assertInstanceOf(UriType.class, view.getUrlElement()); + assertEquals("https://example.org/ViewDefinition/patients", view.getUrl()); + } + + @Test + void testSetUrlWithNullValue() { + // setUrl(null) clears the element, leaving the field null so getUrl() returns null. + final ViewDefinitionResource view = new ViewDefinitionResource(); + view.setUrl("https://example.org/ViewDefinition/patients"); + view.setUrl(null); + + assertNull(view.getUrlElement()); + assertNull(view.getUrl()); + assertFalse(view.hasUrlElement()); + } + + @Test + void testSetVersionWithStringValue() { + // setVersion(String) wraps the value in a StringType and getVersion() unwraps it. + final ViewDefinitionResource view = new ViewDefinitionResource(); + view.setVersion("2.0"); + + assertTrue(view.hasVersionElement()); + assertInstanceOf(org.hl7.fhir.r4.model.StringType.class, view.getVersionElement()); + assertEquals("2.0", view.getVersion()); + } + + @Test + void testSetVersionWithNullValue() { + // setVersion(null) clears the element, leaving the field null so getVersion() returns null. + final ViewDefinitionResource view = new ViewDefinitionResource(); + view.setVersion("2.0"); + view.setVersion(null); + + assertNull(view.getVersionElement()); + assertNull(view.getVersion()); + assertFalse(view.hasVersionElement()); + } + + @Test + void testCopyRetainsUrlAndVersion() { + // copy() must duplicate the url and version elements when they are present. + final ViewDefinitionResource original = createSimpleViewDefinition(); + original.setUrl("https://example.org/ViewDefinition/patients"); + original.setVersion("2.0"); + + final ViewDefinitionResource copy = (ViewDefinitionResource) original.copy(); + + assertTrue(original.equalsDeep(copy)); + assertEquals("https://example.org/ViewDefinition/patients", copy.getUrl()); + assertEquals("2.0", copy.getVersion()); + } + + @Test + void testIsEmptyConsidersUrlAndVersion() { + // A populated url or version makes the resource non-empty. + final ViewDefinitionResource withUrl = new ViewDefinitionResource(); + withUrl.setUrl("https://example.org/ViewDefinition/patients"); + assertFalse(withUrl.isEmpty()); + + final ViewDefinitionResource withVersion = new ViewDefinitionResource(); + withVersion.setVersion("1.0"); + assertFalse(withVersion.isEmpty()); + + // Non-null but empty url and version elements leave the resource empty. + final ViewDefinitionResource emptyElements = new ViewDefinitionResource(); + emptyElements.setUrlElement(new UriType()); + emptyElements.setVersionElement(new org.hl7.fhir.r4.model.StringType()); + assertTrue(emptyElements.isEmpty()); + } } diff --git a/fhirpath/pom.xml b/fhirpath/pom.xml index 2faea92282..42b6ebde1e 100644 --- a/fhirpath/pom.xml +++ b/fhirpath/pom.xml @@ -26,7 +26,7 @@ au.csiro.pathling pathling - 9.8.0 + 9.9.0-SNAPSHOT fhirpath jar diff --git a/lib/R/pom.xml b/lib/R/pom.xml index 32d31f5990..07a9a02fdc 100644 --- a/lib/R/pom.xml +++ b/lib/R/pom.xml @@ -26,7 +26,7 @@ au.csiro.pathling pathling - 9.8.0 + 9.9.0-SNAPSHOT ../../pom.xml r diff --git a/lib/python/pom.xml b/lib/python/pom.xml index a2b455f3ed..b75c429029 100644 --- a/lib/python/pom.xml +++ b/lib/python/pom.xml @@ -26,7 +26,7 @@ au.csiro.pathling pathling - 9.8.0 + 9.9.0-SNAPSHOT ../../pom.xml python diff --git a/library-api/pom.xml b/library-api/pom.xml index 5389270f9f..c79abab72a 100644 --- a/library-api/pom.xml +++ b/library-api/pom.xml @@ -26,7 +26,7 @@ pathling au.csiro.pathling - 9.8.0 + 9.9.0-SNAPSHOT library-api jar diff --git a/library-runtime/pom.xml b/library-runtime/pom.xml index fe2083e4d5..55d9cdb292 100644 --- a/library-runtime/pom.xml +++ b/library-runtime/pom.xml @@ -26,7 +26,7 @@ pathling au.csiro.pathling - 9.8.0 + 9.9.0-SNAPSHOT library-runtime jar @@ -121,6 +121,20 @@ ** + + + jakarta.servlet:jakarta.servlet-api + + jakarta/servlet/SingleThreadModel.class + jakarta/servlet/http/HttpSessionContext.class + jakarta/servlet/http/HttpUtils.class + + 4.0.0 au.csiro.pathling pathling - 9.8.0 + 9.9.0-SNAPSHOT pom Pathling diff --git a/server/.trivyignore b/server/.trivyignore index 1ffa14ece8..af71166b21 100644 --- a/server/.trivyignore +++ b/server/.trivyignore @@ -88,3 +88,17 @@ CVE-2026-43869 # W3C Baggage propagators; the API jar is present only for HAPI FHIR instrumentation # annotations, so the vulnerable Baggage parsing code path is never reached. CVE-2026-45292 + +# okhttp and ini4j are transitive dependencies of spark-hadoop-cloud (via the +# Hadoop Aliyun/GCS cloud connectors), not on Pathling's request path. okhttp's +# fix requires a 3.x->4.x major jump untested against Hadoop 3.4.1; ini4j has no +# fixed release. Neither vulnerable code path is reachable in our usage. +CVE-2021-0341 +CVE-2022-41404 + +# lz4-java native XXHash JVM crash on invalid byte array ranges — a Spark transitive used +# for internal shuffle and block compression. Spark passes only its own valid buffer ranges, +# so the invalid-range path is not reachable from user input. There is no fixed release for +# the org.lz4 coordinate; the fix exists only in the renamed at.yawk.lz4 fork, which Spark +# does not use. +CVE-2026-59949 diff --git a/server/CONTRIBUTING.md b/server/CONTRIBUTING.md index 1cfa008653..227d7b1af0 100644 --- a/server/CONTRIBUTING.md +++ b/server/CONTRIBUTING.md @@ -25,6 +25,22 @@ To install the required dependencies: mvn clean install -pl library-runtime -am ``` +### Building the core from a server release branch + +Run that command from a checkout of the core release branch that the server's +`pathling.version` points at, not from a `release/server/*` branch. The server +branch carries its own copy of the root `pom.xml`, and because the two branches +are versioned independently it drifts behind the core release branch. Building +`library-runtime` from the server branch can therefore produce an artifact whose +bundled dependencies are older than the ones the server source expects, and the +server then fails to compile with a missing symbol from a transitive dependency. + +CI does not see this, because it resolves the `SNAPSHOT` published from the core +release branch. Locally the problem is also sticky: the bad build installs itself +into `~/.m2` under the same coordinates, so it silently replaces the artifact +resolved from CI and affects every other checkout on the machine until it is +rebuilt from the right branch. + ## Docker image The server includes a `docker` profile for building and deploying Docker images. diff --git a/server/pom.xml b/server/pom.xml index 6f54140b82..c065095fd7 100644 --- a/server/pom.xml +++ b/server/pom.xml @@ -24,7 +24,7 @@ au.csiro.pathling server - 2.0.1 + 3.0.0-SNAPSHOT Pathling Server @@ -36,17 +36,17 @@ 21 UTF-8 1 - 9.7.1 + 9.9.0-SNAPSHOT 8.10.0 - 3.5.14 + 3.5.16 4.0.2 3.4.1 2.13 4.0.0 - 4.1.133.Final + 4.1.136.Final false false - 2.18.6 + 2.22.1 1.0.0 2.0.17 3.4.6 @@ -54,8 +54,19 @@ latest amazoncorretto:21 2.40.3 - 1.0.4 + 1.1.0 1.0.0 + + + --add-exports=java.base/sun.nio.ch=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/sun.util.calendar=ALL-UNNAMED + + ${pathling.runtime.jvmModuleOpts} --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.lang.reflect=ALL-UNNAMED --add-opens=java.base/java.io=ALL-UNNAMED --add-opens=java.base/java.nio=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.base/sun.nio.ch=ALL-UNNAMED --add-opens=java.base/sun.nio.cs=ALL-UNNAMED --add-opens=java.base/sun.security.action=ALL-UNNAMED @@ -161,6 +172,14 @@ hadoop-aws ${pathling.hadoopVersion} + + + org.apache.spark + spark-hadoop-cloud_${pathling.scalaVersion} + ${pathling.sparkVersion} + software.amazon.awssdk bundle @@ -385,13 +404,9 @@ pom import - - org.springframework.boot - spring-boot-dependencies - ${pathling.springBootVersion} - pom - import - + com.fasterxml.jackson jackson-bom @@ -400,9 +415,11 @@ import - jakarta.servlet - jakarta.servlet-api - 6.0.0 + org.springframework.boot + spring-boot-dependencies + ${pathling.springBootVersion} + pom + import @@ -411,19 +428,20 @@ 1.12.1 + to a HAPI release that bundles org.hl7.fhir.* >= 6.9.10 natively. --> ca.uhn.hapi.fhir org.hl7.fhir.r4 - 6.9.7 + 6.9.10 ca.uhn.hapi.fhir org.hl7.fhir.utilities - 6.9.7 + 6.9.10 + ${pathling.runtime.jvmModuleOpts} @@ -600,22 +620,7 @@ false - @{argLine} - --add-opens java.base/java.lang=ALL-UNNAMED - --add-opens java.base/java.lang.invoke=ALL-UNNAMED - --add-opens java.base/java.lang.reflect=ALL-UNNAMED - --add-opens java.base/java.io=ALL-UNNAMED - --add-opens java.base/java.net=ALL-UNNAMED - --add-opens java.base/java.nio=ALL-UNNAMED - --add-opens java.base/java.util=ALL-UNNAMED - --add-opens java.base/java.util.concurrent=ALL-UNNAMED - --add-opens java.base/java.util.concurrent.atomic=ALL-UNNAMED - --add-opens java.base/sun.nio.ch=ALL-UNNAMED - --add-opens java.base/sun.nio.cs=ALL-UNNAMED - --add-opens java.base/sun.security.action=ALL-UNNAMED - --add-opens java.base/sun.util.calendar=ALL-UNNAMED - --add-exports java.base/sun.nio.ch=ALL-UNNAMED - + @{argLine} ${pathling.test.jvmModuleOpts} @@ -638,22 +643,7 @@ junit.jupiter.execution.parallel.enabled=true - @{argLine} - --add-opens java.base/java.lang=ALL-UNNAMED - --add-opens java.base/java.lang.invoke=ALL-UNNAMED - --add-opens java.base/java.lang.reflect=ALL-UNNAMED - --add-opens java.base/java.io=ALL-UNNAMED - --add-opens java.base/java.net=ALL-UNNAMED - --add-opens java.base/java.nio=ALL-UNNAMED - --add-opens java.base/java.util=ALL-UNNAMED - --add-opens java.base/java.util.concurrent=ALL-UNNAMED - --add-opens java.base/java.util.concurrent.atomic=ALL-UNNAMED - --add-opens java.base/sun.nio.ch=ALL-UNNAMED - --add-opens java.base/sun.nio.cs=ALL-UNNAMED - --add-opens java.base/sun.security.action=ALL-UNNAMED - --add-opens java.base/sun.util.calendar=ALL-UNNAMED - --add-exports java.base/sun.nio.ch=ALL-UNNAMED - + @{argLine} ${pathling.test.jvmModuleOpts} @@ -785,6 +775,10 @@ org.apache.maven.plugins maven-failsafe-plugin + system-test @@ -792,32 +786,34 @@ integration-test verify + + ${project.build.outputDirectory} + + **/*.java + + SystemTest + + ${git.commit.id} + + https://auth.ontoserver.csiro.au/auth/realms/aehrc + + + pathling-test + + + openid user/*.* + + + https://tx.ontoserver.csiro.au/fhir + + + ${pathling.fhirServerDockerRepo} + + system-test + + - - ${project.build.outputDirectory} - - **/*.java - - SystemTest - - ${git.commit.id} - - https://auth.ontoserver.csiro.au/auth/realms/aehrc - - pathling-test - - openid user/*.* - - - https://tx.ontoserver.csiro.au/fhir - - - ${pathling.fhirServerDockerRepo} - - system-test - - com.google.cloud.tools @@ -846,12 +842,23 @@ - - --add-exports=java.base/sun.nio.ch=ALL-UNNAMED - --add-opens=java.base/java.net=ALL-UNNAMED - --add-opens=java.base/sun.util.calendar=ALL-UNNAMED - + + + /usr/bin/entrypoint.sh + + + src/main/jib + + + /usr/bin/entrypoint.sh + 755 + + + @@ -870,6 +877,28 @@ + + + org.codehaus.mojo + exec-maven-plugin + + + entrypoint-dispatch-test + test + + exec + + + ${skipTests} + bash + + ${project.basedir}/src/test/sh/entrypoint-test.sh + + + + + @@ -881,6 +910,10 @@ org.apache.maven.plugins maven-failsafe-plugin + system-test @@ -888,32 +921,34 @@ integration-test verify + + ${project.build.outputDirectory} + + **/*.java + + SystemTest + + ${git.commit.id} + + https://auth.ontoserver.csiro.au/auth/realms/aehrc + + + pathling-test + + + openid user/*.* + + + https://tx.ontoserver.csiro.au/fhir + + + ${pathling.fhirServerDockerRepo} + + system-test + + - - ${project.build.outputDirectory} - - **/*.java - - SystemTest - - ${git.commit.id} - - https://auth.ontoserver.csiro.au/auth/realms/aehrc - - pathling-test - - openid user/*.* - - - https://tx.ontoserver.csiro.au/fhir - - - ${pathling.fhirServerDockerRepo} - - system-test - - com.google.cloud.tools @@ -937,12 +972,23 @@ ${pathling.fhirServerDockerRepo}:${project.version} - - --add-exports=java.base/sun.nio.ch=ALL-UNNAMED - --add-opens=java.base/java.net=ALL-UNNAMED - --add-opens=java.base/sun.util.calendar=ALL-UNNAMED - + + + /usr/bin/entrypoint.sh + + + src/main/jib + + + /usr/bin/entrypoint.sh + 755 + + + @@ -961,6 +1007,28 @@ + + + org.codehaus.mojo + exec-maven-plugin + + + entrypoint-dispatch-test + test + + exec + + + ${skipTests} + bash + + ${project.basedir}/src/test/sh/entrypoint-test.sh + + + + + diff --git a/server/src/main/java/au/csiro/pathling/Dependencies.java b/server/src/main/java/au/csiro/pathling/Dependencies.java index a5d466bd87..a9bc1c5d7d 100644 --- a/server/src/main/java/au/csiro/pathling/Dependencies.java +++ b/server/src/main/java/au/csiro/pathling/Dependencies.java @@ -21,11 +21,13 @@ import au.csiro.pathling.config.StorageConfiguration; import au.csiro.pathling.encoders.FhirEncoders; import au.csiro.pathling.io.DynamicDeltaSource; +import au.csiro.pathling.io.SchemaMigrator; import au.csiro.pathling.library.PathlingContext; import au.csiro.pathling.library.io.source.QueryableDataSource; import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.parser.IParser; import jakarta.annotation.Nonnull; +import java.util.Set; import lombok.extern.slf4j.Slf4j; import org.apache.spark.sql.SparkSession; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; @@ -85,13 +87,25 @@ static QueryableDataSource deltaLake( serverConfiguration.getStorage().getWarehouseUrl() + "/" + serverConfiguration.getStorage().getDatabaseName(); + // Migrate any tables whose schemas are behind the current encoders before the delegate scans + // the warehouse, so its resource map is built from already-migrated tables. Types that remain + // drifted (flag disabled, or migration failure) are reported to the data source so requests + // against them fail with an actionable error. + final Set driftedTypes = + new SchemaMigrator( + pathlingContext.getSpark(), + pathlingContext.getFhirEncoders(), + databaseLocation, + serverConfiguration.getStorage().getSchemaAutoMerge()) + .migrate(); final QueryableDataSource baseSource = pathlingContext.read().delta(databaseLocation); return new DynamicDeltaSource( baseSource, pathlingContext.getSpark(), databaseLocation, pathlingContext.getFhirEncoders(), - serverConfiguration.getStorage()); + serverConfiguration.getStorage(), + driftedTypes); } @Bean diff --git a/server/src/main/java/au/csiro/pathling/FhirServer.java b/server/src/main/java/au/csiro/pathling/FhirServer.java index 1579f4a368..603817d1ae 100644 --- a/server/src/main/java/au/csiro/pathling/FhirServer.java +++ b/server/src/main/java/au/csiro/pathling/FhirServer.java @@ -19,6 +19,7 @@ import static au.csiro.pathling.utilities.Preconditions.checkPresent; +import au.csiro.pathling.async.JobListProvider; import au.csiro.pathling.async.JobProvider; import au.csiro.pathling.async.JobResultProvider; import au.csiro.pathling.cache.EntityTagInterceptor; @@ -128,6 +129,8 @@ public class FhirServer extends RestfulServer { @Nonnull private final transient Optional jobProvider; + @Nonnull private final transient Optional jobListProvider; + @Nonnull private final transient Optional jobResultProvider; @Nonnull private final transient SystemExportProvider exportProvider; @@ -182,6 +185,14 @@ public class FhirServer extends RestfulServer { private final transient au.csiro.pathling.operations.sqlquery.SqlQueryInstanceRunProvider sqlQueryInstanceRunProvider; + @Nonnull + private final transient au.csiro.pathling.operations.sqlquery.SqlQueryExportProvider + sqlQueryExportProvider; + + @Nonnull + private final transient au.csiro.pathling.operations.sqlquery.SqlQueryInstanceExportProvider + sqlQueryInstanceExportProvider; + /** * Constructs a new FhirServer. * @@ -189,6 +200,7 @@ public class FhirServer extends RestfulServer { * @param configuration the server configuration * @param oidcConfiguration the optional OIDC configuration * @param jobProvider the optional job provider + * @param jobListProvider the optional job list provider * @param jobResultProvider the optional job result provider * @param exportProvider the export provider * @param exportResultProvider the export result provider @@ -213,6 +225,9 @@ public class FhirServer extends RestfulServer { * @param viewDefinitionExportProvider the view definition export provider * @param sqlQueryRunProvider the SQL query run provider * @param sqlQueryInstanceRunProvider the SQL query instance run provider + * @param sqlQueryExportProvider the system-level SQL query export provider + * @param sqlQueryInstanceExportProvider the type-level and instance-level SQL query export + * provider */ @SuppressWarnings("java:S107") public FhirServer( @@ -220,6 +235,7 @@ public FhirServer( @Nonnull final ServerConfiguration configuration, @Nonnull final Optional oidcConfiguration, @Nonnull final Optional jobProvider, + @Nonnull final Optional jobListProvider, @Nonnull final Optional jobResultProvider, @Nonnull final SystemExportProvider exportProvider, @Nonnull final ExportResultProvider exportResultProvider, @@ -245,13 +261,19 @@ public FhirServer( @Nonnull final au.csiro.pathling.operations.sqlquery.SqlQueryRunProvider sqlQueryRunProvider, @Nonnull final au.csiro.pathling.operations.sqlquery.SqlQueryInstanceRunProvider - sqlQueryInstanceRunProvider) { + sqlQueryInstanceRunProvider, + @Nonnull + final au.csiro.pathling.operations.sqlquery.SqlQueryExportProvider sqlQueryExportProvider, + @Nonnull + final au.csiro.pathling.operations.sqlquery.SqlQueryInstanceExportProvider + sqlQueryInstanceExportProvider) { // Pass the FhirContext to the RestfulServer superclass to ensure custom types like // ViewDefinitionResource are recognized when parsing request bodies. super(fhirContext); this.configuration = configuration; this.oidcConfiguration = oidcConfiguration; this.jobProvider = jobProvider; + this.jobListProvider = jobListProvider; this.jobResultProvider = jobResultProvider; this.exportProvider = exportProvider; this.exportResultProvider = exportResultProvider; @@ -276,6 +298,8 @@ public FhirServer( this.viewDefinitionExportProvider = viewDefinitionExportProvider; this.sqlQueryRunProvider = sqlQueryRunProvider; this.sqlQueryInstanceRunProvider = sqlQueryInstanceRunProvider; + this.sqlQueryExportProvider = sqlQueryExportProvider; + this.sqlQueryInstanceExportProvider = sqlQueryInstanceExportProvider; } @Override @@ -297,6 +321,7 @@ protected void initialize() throws ServletException { // Register job providers, if async is enabled. jobProvider.ifPresent(this::registerProvider); + jobListProvider.ifPresent(this::registerProvider); jobResultProvider.ifPresent(this::registerProvider); // Register export providers based on configuration. @@ -400,6 +425,12 @@ protected void initialize() throws ServletException { registerProvider(sqlQueryInstanceRunProvider); } + // Register the SQL query export providers (system, type, and instance levels). + if (ops.isSqlQueryExportEnabled()) { + registerProvider(sqlQueryExportProvider); + registerProvider(sqlQueryInstanceExportProvider); + } + // CORS configuration. configureCors(); diff --git a/server/src/main/java/au/csiro/pathling/async/AsyncAspect.java b/server/src/main/java/au/csiro/pathling/async/AsyncAspect.java index d583e0411c..b8abfec047 100644 --- a/server/src/main/java/au/csiro/pathling/async/AsyncAspect.java +++ b/server/src/main/java/au/csiro/pathling/async/AsyncAspect.java @@ -24,11 +24,14 @@ import au.csiro.pathling.errors.DiagnosticContext; import au.csiro.pathling.errors.ErrorHandlingInterceptor; import au.csiro.pathling.errors.ErrorReportingInterceptor; +import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.server.exceptions.BaseServerResponseException; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; import java.lang.reflect.UndeclaredThrowableException; import java.util.Arrays; import java.util.List; @@ -41,10 +44,13 @@ import org.aspectj.lang.annotation.Around; import org.aspectj.lang.annotation.Aspect; import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.r4.model.CodeType; import org.hl7.fhir.r4.model.OperationOutcome; import org.hl7.fhir.r4.model.OperationOutcome.IssueSeverity; import org.hl7.fhir.r4.model.OperationOutcome.IssueType; import org.hl7.fhir.r4.model.OperationOutcome.OperationOutcomeIssueComponent; +import org.hl7.fhir.r4.model.Parameters; +import org.hl7.fhir.r4.model.StringType; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.core.annotation.Order; import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor; @@ -134,14 +140,29 @@ protected IBaseResource maybeExecuteAsynchronously( // PreAsyncValidation. Set some values to prevent NPEs. result = new PreAsyncValidationResult<>(new Object(), List.of()); } - processRequestAsynchronously(joinPoint, requestDetails, result, spark, asyncSupported); + final Job job = + processRequestAsynchronously(joinPoint, requestDetails, result, spark, asyncSupported); + + if (asyncSupported.pattern() == AsyncPattern.STANDARD_ASYNC_PATTERN) { + // Under the HL7 Asynchronous Interaction Request Pattern, the kick-off body is a Parameters + // acknowledgement (status=accepted, exportId) returned with a 202 status, rather than an + // OperationOutcome. The Content-Location header is already set by + // processRequestAsynchronously. + final HttpServletResponse response = requestDetails.getServletResponse(); + if (response != null) { + response.setStatus(Constants.STATUS_HTTP_202_ACCEPTED); + } + return buildAcceptedAcknowledgement(job.getId()); + } + + // FHIR Bulk Data operations keep the existing OperationOutcome kick-off body. throw new ProcessingNotCompletedException("Accepted", buildOperationOutcome(result)); } else { return (IBaseResource) joinPoint.proceed(); } } - private void processRequestAsynchronously( + private Job processRequestAsynchronously( @Nonnull final ProceedingJoinPoint joinPoint, @Nonnull final ServletRequestDetails requestDetails, @Nonnull final PreAsyncValidationResult preAsyncValidationResult, @@ -170,6 +191,11 @@ private void processRequestAsynchronously( final Future result = executor.submit( () -> { + // Resolve the job once, up front. See removeFilesIfOwned for why the + // clean-up on the way out cannot look it up again, and for what an empty + // result here means. + final Job currentJob = jobRegistry.get(jobId); + boolean failed = false; try { diagnosticContext.configureScope(true); SecurityContextHolder.getContext().setAuthentication(authentication); @@ -179,13 +205,13 @@ private void processRequestAsynchronously( // access it without // needing to look it up from the servlet request (which may have been // recycled). - final Job currentJob = jobRegistry.get(jobId); if (currentJob != null) { AsyncJobContext.setCurrentJob(currentJob); } return (IBaseResource) joinPoint.proceed(); } catch (final Throwable e) { + failed = true; // Unwrap the actual exception from the aspect proxy wrapper, if needed. final Throwable actualEx = unwrapFromProxy(e); @@ -204,20 +230,18 @@ private void processRequestAsynchronously( ErrorReportingInterceptor.getReportableError(convertedError) .getMessage()); } - // Any (partial) files may be deleted if an unexpected error was thrown - // during the processing - jobProvider.deleteJobFiles(jobId); throw new IllegalStateException( "Problem processing request asynchronously", actualEx); } finally { AsyncJobContext.clear(); cleanUpAfterJob(spark, jobId); + removeFilesIfOwned(jobId, currentJob, failed); } }); final Optional ownerId = getCurrentUserId(authentication); final Job newJob = new Job<>(jobId, operation, result, ownerId); newJob.setPreAsyncValidationResult(preAsyncValidationResult.result()); - newJob.setRedirectOnComplete(asyncSupported.redirectOnComplete()); + newJob.setPattern(asyncSupported.pattern()); return newJob; }); final HttpServletResponse response = requestDetails.getServletResponse(); @@ -229,6 +253,22 @@ private void processRequestAsynchronously( final String asyncEtag = "W/\"~" + serverInstanceId.getId() + "." + hashJobId(job.getId()) + "\""; response.setHeader("ETag", asyncEtag); + return job; + } + + /** + * Builds the SQL on FHIR kick-off acknowledgement: a {@code Parameters} resource carrying {@code + * status=accepted} and the {@code exportId}. + * + * @param jobId the server-assigned job identifier + * @return the acknowledgement Parameters resource + */ + @Nonnull + private static Parameters buildAcceptedAcknowledgement(@Nonnull final String jobId) { + final Parameters parameters = new Parameters(); + parameters.addParameter().setName("status").setValue(new CodeType("accepted")); + parameters.addParameter().setName("exportId").setValue(new StringType(jobId)); + return parameters; } /** @@ -262,6 +302,48 @@ private ServletRequestDetails getServletRequestDetails(@Nonnull final Object[] a + " parameter")); } + /** + * Removes the job's output directory if the job's own thread owns the removal, as it unwinds. + * + *

That thread is the last party to touch the output, so it owns the removal whenever a client + * has already asked for the job to be deleted. A job that failed also removes its own partial + * output, whether or not a client asked for it to be deleted. + * + *

The job is passed in rather than looked up, because by this point a {@code DELETE} may + * already have removed it from the registry, and a lookup would then find nothing and skip a + * removal that is owed. + * + *

A job that was already absent when the task started owns its removal too. The task resolves + * the job as its first statement, and {@link JobRegistry#getOrCreate} holds the same monitor as + * {@link JobRegistry#get} until the registration completes, so the lookup cannot observe the gap + * before registration. The only way it can come back empty is a {@code DELETE} that removed the + * job in the narrow window between the task entering its body and that first statement running. + * Such a request cannot have taken the claim, because the work had not terminated when it asked, + * and it is the only thing in the server that removes a job from the registry. Removing an absent + * directory is a no-op, so nothing is lost if that reasoning is ever widened. + * + *

Nothing is thrown from here, because this runs in a {@code finally} block where an + * incidental exception would replace the job's own. + * + * @param jobId the identifier of the job that has just finished + * @param job the job, or null if it was not in the registry when the task started + * @param failed whether the job's work threw + */ + private void removeFilesIfOwned( + @Nonnull final String jobId, @Nullable final Job job, final boolean failed) { + // markTerminatedAndClaim is what records that the job's thread has finished, so it has to be + // evaluated before any short-circuiting on the failure flag. + final boolean claimedDeletion = job == null || job.markTerminatedAndClaim(); + if (!claimedDeletion && !failed) { + return; + } + try { + jobProvider.deleteJobFiles(jobId); + } catch (final IOException | RuntimeException e) { + JobProvider.reportFileRemovalFailure(jobId, e); + } + } + private void cleanUpAfterJob(@Nonnull final SparkSession spark, @Nonnull final String requestId) { spark.sparkContext().clearJobGroup(); // Clean up the stage mappings. diff --git a/server/src/main/java/au/csiro/pathling/async/AsyncPattern.java b/server/src/main/java/au/csiro/pathling/async/AsyncPattern.java new file mode 100644 index 0000000000..e9582194a0 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/async/AsyncPattern.java @@ -0,0 +1,47 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.async; + +/** + * The asynchronous wire contract that an operation follows. The two patterns differ in the kick-off + * acknowledgement body returned with the {@code 202 Accepted} response and in how a completed job + * is delivered when polled via {@code $job}. + * + *

The redirect-based contract is the HL7 Asynchronous Interaction Request Pattern, defined by + * the HL7 R6 API Incubator: Asynchronous + * Interaction Request Pattern. + * + * @author John Grimes + */ +public enum AsyncPattern { + + /** + * The HL7 Asynchronous Interaction Request Pattern. Kick-off returns a {@code Parameters} + * acknowledgement (with {@code status=accepted} and an {@code exportId}), and a completed job + * returns {@code 303 See Other} pointing at a separate {@code $job-result} endpoint. + */ + STANDARD_ASYNC_PATTERN, + + /** + * The FHIR Bulk Data pattern used by {@code $export}/{@code $import}. Kick-off returns an {@code + * OperationOutcome}, and a completed job returns the result manifest inline with {@code 200 OK}. + * This is the default. + */ + BULK_DATA +} diff --git a/server/src/main/java/au/csiro/pathling/async/AsyncSupported.java b/server/src/main/java/au/csiro/pathling/async/AsyncSupported.java index ab52a4ad33..3f6f37318d 100644 --- a/server/src/main/java/au/csiro/pathling/async/AsyncSupported.java +++ b/server/src/main/java/au/csiro/pathling/async/AsyncSupported.java @@ -36,10 +36,13 @@ public @interface AsyncSupported { /** - * When true, completed jobs return 303 See Other with a redirect to the result endpoint, rather - * than returning the result inline. This follows the SQL on FHIR unify-async specification. + * The asynchronous wire contract this operation follows. Selecting {@link + * AsyncPattern#STANDARD_ASYNC_PATTERN} (the HL7 Asynchronous Interaction Request Pattern, spec) + * makes a completed job return 303 See Other with a redirect to the result endpoint, rather than + * returning the result inline. Defaults to {@link AsyncPattern#BULK_DATA}. * - * @return true if completed jobs should redirect to the result endpoint + * @return the asynchronous pattern for this operation */ - boolean redirectOnComplete() default false; + AsyncPattern pattern() default AsyncPattern.BULK_DATA; } diff --git a/server/src/main/java/au/csiro/pathling/async/Job.java b/server/src/main/java/au/csiro/pathling/async/Job.java index e1db1babea..cffdf8b152 100644 --- a/server/src/main/java/au/csiro/pathling/async/Job.java +++ b/server/src/main/java/au/csiro/pathling/async/Job.java @@ -19,9 +19,11 @@ import jakarta.annotation.Nonnull; import jakarta.servlet.http.HttpServletResponse; +import java.time.Instant; import java.util.Optional; import java.util.concurrent.Future; import java.util.function.Consumer; +import lombok.AccessLevel; import lombok.Getter; import lombok.Setter; import lombok.ToString; @@ -56,6 +58,12 @@ public interface JobTag {} /** The identifier of the user who owns this job, if authenticated. */ @Nonnull private final Optional ownerId; + /** + * The time at which this job was created (kick-off time). Used to populate the SQL on FHIR export + * manifest's {@code exportStartTime} and to compute {@code exportDuration}. + */ + @Nonnull private final Instant startTime; + /** The total number of stages in this job, used to calculate progress percentage. */ private int totalStages; @@ -68,14 +76,36 @@ public interface JobTag {} /** A consumer that modifies the HTTP response for this job, such as adding headers. */ @Setter private Consumer responseModification; - /** Indicates whether this job has been marked for deletion. */ - @Setter private boolean markedAsDeleted; + /** + * Indicates whether a client has asked for this job to be deleted. Guarded by this instance's + * monitor and only ever set through {@link #markDeletedAndClaim()}. + */ + @Getter(AccessLevel.NONE) + private boolean markedAsDeleted; + + /** + * Indicates whether the thread executing this job's work has finished unwinding. This is not the + * same as the job's future being done: cancelling a future whose task has already started reports + * the future as done immediately, while the work carries on. Guarded by this instance's monitor. + */ + @Getter(AccessLevel.NONE) + private boolean terminated; + + /** + * Indicates whether some party has taken responsibility for removing this job's output directory. + * Once set it never clears. Guarded by this instance's monitor. + */ + @Getter(AccessLevel.NONE) + private boolean deletionClaimed; /** - * When true, completed jobs return 303 See Other with redirect to result endpoint, rather than - * returning the result inline. This follows the SQL on FHIR unify-async specification. + * The asynchronous wire contract this job follows. Under {@link + * AsyncPattern#STANDARD_ASYNC_PATTERN} (the HL7 Asynchronous Interaction Request Pattern, spec) + * a completed job returns 303 See Other with a redirect to the result endpoint, rather than + * returning the result inline. Defaults to {@link AsyncPattern#BULK_DATA} and is never null. */ - @Setter private boolean redirectOnComplete; + @Setter private AsyncPattern pattern = AsyncPattern.BULK_DATA; /** * The last calculated progress percentage. When a job is at 100% that does not always indicate @@ -102,6 +132,7 @@ public Job( this.operation = operation; this.result = result; this.ownerId = ownerId; + this.startTime = Instant.now(); this.responseModification = httpServletResponse -> {}; } @@ -146,4 +177,83 @@ public void setPreAsyncValidationResult(final Object preAsyncValidationResult) { public boolean isCancelled() { return result.isCancelled(); } + + /** + * Checks whether a client has asked for this job to be deleted. + * + * @return true if the job has been marked for deletion, false otherwise + */ + public synchronized boolean isMarkedAsDeleted() { + return markedAsDeleted; + } + + /** + * Checks whether the thread executing this job's work has finished unwinding. + * + * @return true if the work has terminated, false otherwise + */ + public synchronized boolean isTerminated() { + return terminated; + } + + /** + * Records that a client has asked for this job to be deleted, and determines whether the caller + * owns the removal of the job's output directory. + * + *

Called by the request handling the deletion. A {@code true} return obliges the caller to + * remove the directory; a {@code false} return means the job's own thread has not finished yet + * and will perform the removal as it exits. + * + * @return true if the caller has taken responsibility for removing the job's output directory + */ + public synchronized boolean markDeletedAndClaim() { + markedAsDeleted = true; + return terminated && claim(); + } + + /** + * Records that the thread executing this job's work has finished unwinding, and determines + * whether that thread owns the removal of the job's output directory. + * + *

Called by the job's own thread as it exits. A {@code true} return obliges the caller to + * remove the directory; a {@code false} return means either that no client has asked for the job + * to be deleted, or that the request handling the deletion has already removed it. + * + * @return true if the caller has taken responsibility for removing the job's output directory + */ + public synchronized boolean markTerminatedAndClaim() { + markTerminated(); + return markedAsDeleted && claim(); + } + + /** + * Records that the thread executing this job's work has finished unwinding, without contending + * for the removal of the job's output directory. + * + *

Called at registration time for jobs whose work is not run by the asynchronous request + * machinery. No thread will ever signal termination for such a job, so marking it terminated up + * front is what allows a deletion request to perform its own clean-up rather than deferring it to + * a thread that never arrives. + */ + public synchronized void markTerminated() { + terminated = true; + } + + /** + * Takes the single-use claim on removing this job's output directory, if it is still free. + * + *

Both entry points call this from inside the instance monitor, so their bodies are totally + * ordered. Whichever runs second observes the flag written by the first, which is what guarantees + * that at least one party claims once both have been called; this flag is what guarantees that at + * most one does. + * + * @return true if the claim was free and has now been taken by the caller + */ + private synchronized boolean claim() { + if (deletionClaimed) { + return false; + } + deletionClaimed = true; + return true; + } } diff --git a/server/src/main/java/au/csiro/pathling/async/JobListProvider.java b/server/src/main/java/au/csiro/pathling/async/JobListProvider.java new file mode 100644 index 0000000000..3605120595 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/async/JobListProvider.java @@ -0,0 +1,165 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.async; + +import static au.csiro.pathling.security.SecurityAspect.checkHasAuthority; +import static au.csiro.pathling.security.SecurityAspect.getCurrentUserId; + +import au.csiro.pathling.config.ServerConfiguration; +import au.csiro.pathling.security.PathlingAuthority; +import ca.uhn.fhir.rest.annotation.Operation; +import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import jakarta.servlet.http.HttpServletResponse; +import java.util.Comparator; +import java.util.Date; +import java.util.Optional; +import lombok.extern.slf4j.Slf4j; +import org.hl7.fhir.r4.model.CodeType; +import org.hl7.fhir.r4.model.InstantType; +import org.hl7.fhir.r4.model.IntegerType; +import org.hl7.fhir.r4.model.Parameters; +import org.hl7.fhir.r4.model.Parameters.ParametersParameterComponent; +import org.hl7.fhir.r4.model.StringType; +import org.hl7.fhir.r4.model.UriType; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.stereotype.Component; + +/** + * Provides the system-level {@code $jobs} operation, which lists the asynchronous jobs held in the + * in-memory {@link JobRegistry}. The list is owner-scoped when authorisation is enabled and + * contains every registered job when it is disabled. + * + * @author John Grimes + */ +@Component +@ConditionalOnProperty(prefix = "pathling", name = "async.enabled", havingValue = "true") +@Slf4j +public class JobListProvider { + + /** The authority and operation name required to list jobs. */ + private static final String JOBS_OPERATION = "jobs"; + + @Nonnull private final ServerConfiguration configuration; + @Nonnull private final JobRegistry jobRegistry; + + /** + * Creates a new JobListProvider. + * + * @param configuration the server configuration, for determining if authorisation is enabled + * @param jobRegistry the registry to enumerate jobs from + */ + public JobListProvider( + @Nonnull final ServerConfiguration configuration, @Nonnull final JobRegistry jobRegistry) { + this.configuration = configuration; + this.jobRegistry = jobRegistry; + } + + /** + * Lists the jobs owned by the caller as a {@link Parameters} resource, newest first. When + * authorisation is enabled the caller must hold the {@code operation:jobs} authority and only + * their own jobs are returned; when it is disabled every registered job is returned. + * + * @param requestDetails the request details, used to build absolute job status URLs + * @param response the HTTP response, used to mark the list as non-cacheable + * @return a {@link Parameters} resource with one repeating {@code job} parameter per job + */ + @Operation(name = "$jobs", idempotent = true) + public Parameters jobs( + @Nonnull final ServletRequestDetails requestDetails, + @Nullable final HttpServletResponse response) { + log.debug("Received $jobs request"); + + final boolean authEnabled = configuration.getAuth().isEnabled(); + final Optional currentUserId; + if (authEnabled) { + // The caller must hold the authority for the list operation itself. + checkHasAuthority(PathlingAuthority.operationAccess(JOBS_OPERATION)); + final Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + currentUserId = getCurrentUserId(authentication); + } else { + currentUserId = Optional.empty(); + } + + // The list is a live snapshot of transient state, so it must not be cached. + if (response != null) { + response.setHeader("Cache-Control", "no-cache"); + } + + final String fhirServerBase = requestDetails.getFhirServerBase(); + final Parameters result = new Parameters(); + jobRegistry.allJobs().stream() + .filter(job -> isVisibleToCaller(job, authEnabled, currentUserId)) + .sorted(Comparator.comparing((Job job) -> job.getStartTime()).reversed()) + .forEach(job -> addJobParameter(result, job, fhirServerBase)); + return result; + } + + /** + * Determines whether a job should appear in the caller's list. All jobs are visible when + * authorisation is disabled; otherwise only jobs owned by the caller's subject are, and a caller + * without a subject sees none. + * + * @param job the job to test + * @param authEnabled whether authorisation is enabled + * @param currentUserId the caller's subject, when known + * @return true if the job should be listed for this caller + */ + private static boolean isVisibleToCaller( + @Nonnull final Job job, + final boolean authEnabled, + @Nonnull final Optional currentUserId) { + if (!authEnabled) { + return true; + } + return currentUserId.isPresent() && job.getOwnerId().equals(currentUserId); + } + + /** + * Appends a single {@code job} parameter, with its parts, to the response. + * + * @param result the Parameters resource being built + * @param job the job to project + * @param fhirServerBase the absolute FHIR server base, for the status URL + */ + private static void addJobParameter( + @Nonnull final Parameters result, + @Nonnull final Job job, + @Nonnull final String fhirServerBase) { + final JobStatus status = JobStatus.fromResult(job.getResult()); + final ParametersParameterComponent jobParam = result.addParameter().setName("job"); + jobParam.addPart().setName("id").setValue(new StringType(job.getId())); + jobParam.addPart().setName("operation").setValue(new CodeType(job.getOperation())); + jobParam.addPart().setName("status").setValue(new CodeType(status.getCode())); + // Progress is only meaningful, and only free of a divide-by-zero, for in-progress jobs whose + // total stage count is known. + if (status == JobStatus.IN_PROGRESS && job.getTotalStages() > 0) { + jobParam.addPart().setName("progress").setValue(new IntegerType(job.getProgressPercentage())); + } + final InstantType startTime = new InstantType(Date.from(job.getStartTime())); + startTime.setTimeZoneZulu(true); + jobParam.addPart().setName("startTime").setValue(startTime); + jobParam + .addPart() + .setName("url") + .setValue(new UriType(fhirServerBase + "/$job?id=" + job.getId())); + } +} diff --git a/server/src/main/java/au/csiro/pathling/async/JobProvider.java b/server/src/main/java/au/csiro/pathling/async/JobProvider.java index b4d464d7ff..98ab508e35 100644 --- a/server/src/main/java/au/csiro/pathling/async/JobProvider.java +++ b/server/src/main/java/au/csiro/pathling/async/JobProvider.java @@ -24,6 +24,7 @@ import au.csiro.pathling.config.ServerConfiguration; import au.csiro.pathling.errors.AccessDeniedError; import au.csiro.pathling.errors.ErrorHandlingInterceptor; +import au.csiro.pathling.errors.ErrorReportingInterceptor; import au.csiro.pathling.errors.ResourceNotFoundError; import au.csiro.pathling.io.JobDirectoryFileSystem; import au.csiro.pathling.security.PathlingAuthority; @@ -40,8 +41,7 @@ import java.util.concurrent.ExecutionException; import java.util.regex.Pattern; import lombok.extern.slf4j.Slf4j; -import org.apache.hadoop.fs.FileSystem; -import org.apache.hadoop.fs.Path; +import org.apache.spark.sql.SparkSession; import org.hl7.fhir.instance.model.api.IBaseOperationOutcome; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.r4.model.OperationOutcome; @@ -74,6 +74,7 @@ public class JobProvider { @Nonnull private final JobRegistry jobRegistry; @Nonnull private final JobDirectoryFileSystem jobDirectoryFileSystem; + @Nonnull private final SparkSession spark; /** * Creates a new JobProvider. @@ -82,14 +83,17 @@ public class JobProvider { * @param jobRegistry the {@link JobRegistry} used to keep track of running jobs * @param jobDirectoryFileSystem the {@link JobDirectoryFileSystem} used to resolve and delete * per-job directories on the warehouse file system + * @param spark the {@link SparkSession} used to cancel the Spark work belonging to a deleted job */ public JobProvider( @Nonnull final ServerConfiguration configuration, @Nonnull final JobRegistry jobRegistry, - @Nonnull final JobDirectoryFileSystem jobDirectoryFileSystem) { + @Nonnull final JobDirectoryFileSystem jobDirectoryFileSystem, + @Nonnull final SparkSession spark) { this.configuration = configuration; this.jobRegistry = jobRegistry; this.jobDirectoryFileSystem = jobDirectoryFileSystem; + this.spark = spark; } /** @@ -99,6 +103,18 @@ public JobProvider( */ public void deleteJob(final String jobId) { final Job job = getJob(jobId); + + if (configuration.getAuth().isEnabled()) { + // Mirror the ownership checks on the GET path: the caller must hold the authority for the + // operation that initiated the job, and must be the job's owner. + checkHasAuthority(PathlingAuthority.operationAccess(job.getOperation())); + final Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + final Optional currentUserId = getCurrentUserId(authentication); + if (!job.getOwnerId().equals(currentUserId)) { + throw new AccessDeniedError("The requested job is not owned by the current user"); + } + } + handleJobDeleteRequest(job); } @@ -162,7 +178,8 @@ public IBaseResource job( private void handleJobDeleteRequest(final Job job) { /* Two possible situations: - - The initial kick-off request is still ongoing -> cancel it and delete the partial files + - The initial kick-off request is still ongoing -> cancel it and let the job's own thread + remove the partial files as it exits - The initial kick-off request is complete (and the client may have already downloaded the files) -> interpret delete request from client as "do no longer need them". Depending on the @@ -176,45 +193,78 @@ private void handleJobDeleteRequest(final Job job) { if (job.isMarkedAsDeleted()) { throw new ResourceNotFoundException("Already deleted this job."); } - job.setMarkedAsDeleted(true); + // Whichever party is last owns the removal of the job's output directory: this request if the + // work has already stopped, otherwise the job's own thread as it exits. Removing it here while + // tasks are still writing into it would leave output behind that nothing ever cleans up. + final boolean removeFilesNow = job.markDeletedAndClaim(); if (!job.getResult().isDone()) { job.getResult().cancel(false); - // Currently, only the files up until "now" will be deleted. Anything that is created between - // the cancel request and the job actually being cancelled by spark will remain on the disk + // Signal Spark directly. Cancelling the future does not interrupt the thread running the job, + // and the stage-event checks in SparkJobListener only reach Spark at the next stage boundary, + // which for a job inside a single long write stage is not until that stage finishes on its + // own. Those checks remain as a backstop. + spark.sparkContext().cancelJobGroup(job.getId()); } - try { - deleteJobFiles(job.getId()); - } catch (final IOException e) { - throw new InternalErrorException("Failed to delete files associated with the job.", e); - } finally { - final boolean removed = jobRegistry.remove(job); - if (removed) { - log.debug("Removed job {} from registry.", job.getId()); - } else { - log.warn( - "Failed to remove job {} from registry. This might in wrong caching results.", - job.getId()); - } + final boolean removalFailed = removeFilesNow && !tryDeleteJobFiles(job.getId()); + final boolean removed = jobRegistry.remove(job); + if (removed) { + log.debug("Removed job {} from registry.", job.getId()); + } else { + log.warn( + "Failed to remove job {} from registry. This might in wrong caching results.", + job.getId()); } throw new ProcessingNotCompletedException( - "The job and its resources will be deleted.", buildDeletionOutcome()); + "The job and its resources will be deleted.", buildDeletionOutcome(removalFailed)); } /** - * Deletes the files associated with a job from the file system. + * Deletes the files associated with a job from the file system. Deleting a directory that does + * not exist is a normal outcome and is not reported as a failure. * * @param jobId the ID of the job whose files should be deleted - * @throws IOException if file deletion fails + * @throws IOException if the directory exists but could not be deleted */ public void deleteJobFiles(final String jobId) throws IOException { - final FileSystem fs = jobDirectoryFileSystem.getFileSystem(); - final Path jobDirToDel = jobDirectoryFileSystem.jobDirectory(jobId); - log.debug("Deleting dir {}", jobDirToDel); - final boolean deleted = fs.delete(jobDirToDel, true); - if (!deleted) { - log.warn("Failed to delete dir {}", jobDirToDel); + log.debug("Deleting job directory for job {}", jobId); + jobDirectoryFileSystem.deleteJobDirectory(jobId); + log.debug("Deleted job directory for job {}", jobId); + } + + /** + * Removes the files associated with a job, reporting a failure rather than propagating it. By the + * time this runs the job has been cancelled, so there is nothing the client can usefully retry. + * + * @param jobId the ID of the job whose files should be removed + * @return true if the removal succeeded, false if it failed + */ + private boolean tryDeleteJobFiles(@Nonnull final String jobId) { + try { + deleteJobFiles(jobId); + return true; + } catch (final IOException e) { + reportFileRemovalFailure(jobId, e); + return false; } - log.debug("Deleted dir {}", jobDirToDel); + } + + /** + * Records a failure to remove a job's output directory, in the server log and in error reporting. + * The operator is the only party who can act on it: the files are orphaned in the warehouse and + * need manual attention. + * + *

Shared with {@link AsyncAspect}, which performs the same removal from the job's own thread + * and has no response left to report the failure on. + * + * @param jobId the ID of the job whose files could not be removed + * @param cause the failure encountered while removing them + */ + public static void reportFileRemovalFailure( + @Nonnull final String jobId, @Nonnull final Throwable cause) { + log.error("Failed to remove the output directory of job {}.", jobId, cause); + ErrorReportingInterceptor.reportExceptionToSentry( + new InternalErrorException( + "Failed to remove the output directory of job %s.".formatted(jobId), cause)); } private IBaseResource handleJobGetRequest( @@ -248,9 +298,11 @@ private static ResourceNotFoundException handleCancelledJob() { /** * Handles a completed job by returning its result or redirecting to the result endpoint. * - *

If the job has {@code redirectOnComplete} enabled (following the SQL on FHIR unify-async - * specification), returns 303 See Other with a Location header pointing to the result endpoint. - * Otherwise, returns the result inline. + *

If the job follows the {@link AsyncPattern#STANDARD_ASYNC_PATTERN} (the HL7 Asynchronous + * Interaction Request Pattern, spec), + * returns 303 See Other with a Location header pointing to the result endpoint. Otherwise, + * returns the result inline. * * @param job The completed job. * @param request The HTTP request for building the result URL. @@ -269,8 +321,9 @@ private IBaseResource handleCompletedJob( setAsyncCacheHeaders(response); } - // If redirect is enabled, return 303 See Other with Location header. - if (job.isRedirectOnComplete() && response != null) { + // Under the HL7 Asynchronous Interaction Request Pattern, return 303 See Other with a + // Location header pointing to the result endpoint. + if (job.getPattern() == AsyncPattern.STANDARD_ASYNC_PATTERN && response != null) { final String resultUrl = buildResultUrl(request, job.getId()); response.setStatus(HttpServletResponse.SC_SEE_OTHER); response.setHeader("Location", resultUrl); @@ -366,13 +419,29 @@ private static void setProgressHeader( } } - private static IBaseOperationOutcome buildDeletionOutcome() { + /** + * Builds the outcome returned when a job is deleted. The informational issue comes first and is + * unchanged, so a client reading only the first issue sees what it always has. + * + * @param removalFailed whether the job's output directory could not be removed + * @return the outcome to attach to the acceptance + */ + @Nonnull + private static IBaseOperationOutcome buildDeletionOutcome(final boolean removalFailed) { final OperationOutcome operationOutcome = new OperationOutcome(); operationOutcome .addIssue() .setCode(IssueType.INFORMATIONAL) .setSeverity(IssueSeverity.INFORMATION) .setDiagnostics("The job and its resources will be deleted."); + if (removalFailed) { + operationOutcome + .addIssue() + .setCode(IssueType.INCOMPLETE) + .setSeverity(IssueSeverity.WARNING) + .setDiagnostics( + "The job's stored files could not be removed and may require manual clean-up."); + } return operationOutcome; } diff --git a/server/src/main/java/au/csiro/pathling/async/JobRegistry.java b/server/src/main/java/au/csiro/pathling/async/JobRegistry.java index cf75ef2a97..f47f06a149 100644 --- a/server/src/main/java/au/csiro/pathling/async/JobRegistry.java +++ b/server/src/main/java/au/csiro/pathling/async/JobRegistry.java @@ -18,11 +18,11 @@ package au.csiro.pathling.async; import au.csiro.pathling.async.Job.JobTag; -import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; import jakarta.annotation.Nonnull; import jakarta.annotation.Nullable; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.UUID; @@ -130,11 +130,11 @@ public synchronized boolean remove(@Nonnull final Job job) { log.warn("Failed to remove job {} from registry.", job.getId()); return false; } + // Jobs registered through register rather than getOrCreate have no tag, so finding no tag entry + // is a normal outcome rather than an inconsistency. final boolean removedFromTags = jobsByTags.values().removeIf(otherJob -> otherJob.equals(job)); if (!removedFromTags) { - throw new InternalErrorException( - "Removed job %s from id map but failed to remove it from tag map." - .formatted(job.getId())); + log.debug("Job {} had no tag entry to remove.", job.getId()); } removedFromRegistryButStillWithSparkJob.add(job.getId()); return true; @@ -149,4 +149,16 @@ public synchronized boolean remove(@Nonnull final Job job) { public boolean removedFromRegistryButStillWithSparkJobContains(final String jobId) { return removedFromRegistryButStillWithSparkJob.contains(jobId); } + + /** + * Returns a point-in-time snapshot of all jobs currently registered. The returned list is a safe + * copy that is not affected by subsequent registrations or removals, and callers cannot mutate + * the registry through it. + * + * @return an immutable list of the currently registered jobs + */ + @Nonnull + public synchronized List> allJobs() { + return List.copyOf(jobsById.values()); + } } diff --git a/server/src/main/java/au/csiro/pathling/async/JobResultProvider.java b/server/src/main/java/au/csiro/pathling/async/JobResultProvider.java index e734c8b044..a53e692649 100644 --- a/server/src/main/java/au/csiro/pathling/async/JobResultProvider.java +++ b/server/src/main/java/au/csiro/pathling/async/JobResultProvider.java @@ -46,8 +46,9 @@ /** * Provides the $job-result operation for retrieving the result of a completed async job. This - * endpoint is used when operations are configured with {@code redirectOnComplete=true}, following - * the SQL on FHIR unify-async specification. + * endpoint is used by operations following the {@link AsyncPattern#STANDARD_ASYNC_PATTERN} (the HL7 + * Asynchronous Interaction Request Pattern, spec). * *

The flow is: 1. Client polls $job endpoint until job completes 2. $job returns 303 See Other * with Location header pointing to $job-result 3. Client fetches result from $job-result endpoint diff --git a/server/src/main/java/au/csiro/pathling/async/JobStatus.java b/server/src/main/java/au/csiro/pathling/async/JobStatus.java new file mode 100644 index 0000000000..577668489b --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/async/JobStatus.java @@ -0,0 +1,91 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.async; + +import jakarta.annotation.Nonnull; +import java.util.concurrent.CancellationException; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Future; + +/** + * The externally visible status of an asynchronous job, derived from the job's {@link Future} + * rather than stored. The {@link Future} is the single source of truth for a job's state. + * + * @author John Grimes + */ +public enum JobStatus { + + /** The job's background work has not yet finished. */ + IN_PROGRESS("in-progress"), + + /** The job's background work finished normally. */ + COMPLETED("completed"), + + /** The job's background work finished by throwing an exception. */ + FAILED("failed"), + + /** The job was cancelled before its background work finished. */ + CANCELLED("cancelled"); + + @Nonnull private final String code; + + JobStatus(@Nonnull final String code) { + this.code = code; + } + + /** + * Returns the stable wire code for this status, used in the {@code $jobs} operation response. + * + * @return the wire code (for example {@code in-progress}) + */ + @Nonnull + public String getCode() { + return code; + } + + /** + * Derives the status of a job from its result {@link Future}. Cancellation is checked first, then + * completion; {@code get()} is only invoked once the future is done, so derivation never blocks. + * + * @param result the job's result future + * @return the derived {@link JobStatus} + */ + @Nonnull + public static JobStatus fromResult(@Nonnull final Future result) { + if (result.isCancelled()) { + return CANCELLED; + } + if (!result.isDone()) { + return IN_PROGRESS; + } + try { + // The future is done and not cancelled, so this returns immediately without blocking. + result.get(); + return COMPLETED; + } catch (final CancellationException e) { + // A cancellation that raced with completion is still a cancellation. + return CANCELLED; + } catch (final ExecutionException e) { + return FAILED; + } catch (final InterruptedException e) { + // Restore the interrupt flag and treat the job as failed rather than swallowing the signal. + Thread.currentThread().interrupt(); + return FAILED; + } + } +} diff --git a/server/src/main/java/au/csiro/pathling/cache/EntityTagInterceptor.java b/server/src/main/java/au/csiro/pathling/cache/EntityTagInterceptor.java index e1b5d9d691..5e4caebbba 100644 --- a/server/src/main/java/au/csiro/pathling/cache/EntityTagInterceptor.java +++ b/server/src/main/java/au/csiro/pathling/cache/EntityTagInterceptor.java @@ -113,9 +113,12 @@ public void checkIncomingTag( return; } - // Skip ETag validation for async endpoints - they use TTL-based caching instead. + // Skip ETag validation for async endpoints - they use TTL-based caching instead. The $jobs + // list is a live snapshot of registry state, not data state, so it must not be validated + // against the database ETag (which would serve a stale 304 when jobs change but data does + // not). final String operation = requestDetails.getOperation(); - if ("$job".equals(operation) || "$result".equals(operation)) { + if ("$job".equals(operation) || "$jobs".equals(operation) || "$result".equals(operation)) { log.debug("Async endpoint {}, skipping ETag validation", operation); setMissResponseHeaders(response); return; diff --git a/server/src/main/java/au/csiro/pathling/config/OperationConfiguration.java b/server/src/main/java/au/csiro/pathling/config/OperationConfiguration.java index ce82e00113..2799fa398b 100644 --- a/server/src/main/java/au/csiro/pathling/config/OperationConfiguration.java +++ b/server/src/main/java/au/csiro/pathling/config/OperationConfiguration.java @@ -73,15 +73,25 @@ public class OperationConfiguration { /** Enables $sqlquery-run operation. */ private boolean sqlQueryRunEnabled = true; + /** Enables $sqlquery-export operation. */ + private boolean sqlQueryExportEnabled = true; + /** Enables $bulk-submit operation. */ private boolean bulkSubmitEnabled = true; /** - * Returns true if any export operation is enabled. + * Returns true if any operation that serves its results through the {@code $result} endpoint is + * enabled. This covers the Bulk Data exports as well as the SQL on FHIR asynchronous export + * operations ({@code $viewdefinition-export} and {@code $sqlquery-export}), all of which write + * downloadable files served by {@code $result}. * - * @return true if system, patient, or group export is enabled + * @return true if any export operation that relies on the {@code $result} endpoint is enabled */ public boolean isAnyExportEnabled() { - return exportEnabled || patientExportEnabled || groupExportEnabled; + return exportEnabled + || patientExportEnabled + || groupExportEnabled + || viewDefinitionExportEnabled + || sqlQueryExportEnabled; } } diff --git a/server/src/main/java/au/csiro/pathling/config/PnpConfiguration.java b/server/src/main/java/au/csiro/pathling/config/PnpConfiguration.java index 8a63d8cf17..dbed4ceda1 100644 --- a/server/src/main/java/au/csiro/pathling/config/PnpConfiguration.java +++ b/server/src/main/java/au/csiro/pathling/config/PnpConfiguration.java @@ -80,4 +80,19 @@ public class PnpConfiguration { * false for security. */ private boolean allowInternalUrls = false; + + /** + * The number of files to download concurrently. Each download is written to storage as it is + * received, so a value higher than the storage can keep up with leaves connections idle waiting + * for their turn to write, risking a socket timeout. Defaults to 4. + */ + private int maxConcurrentDownloads = 4; + + /** + * The number of milliseconds a download may wait for more data before the connection is treated + * as failed. A download that is blocked writing what it has already received is not reading from + * its connection, so this needs to accommodate the slowest write the storage will perform, not + * just network latency. Defaults to 600000 (ten minutes). + */ + private int downloadSocketTimeout = 600_000; } diff --git a/server/src/main/java/au/csiro/pathling/config/SecurityConfiguration.java b/server/src/main/java/au/csiro/pathling/config/SecurityConfiguration.java index 5e07531a43..4aef249cfb 100644 --- a/server/src/main/java/au/csiro/pathling/config/SecurityConfiguration.java +++ b/server/src/main/java/au/csiro/pathling/config/SecurityConfiguration.java @@ -31,6 +31,7 @@ import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; import org.springframework.security.config.annotation.web.configurers.AbstractHttpConfigurer; +import org.springframework.security.config.http.SessionCreationPolicy; import org.springframework.security.oauth2.jwt.JwtDecoder; import org.springframework.security.oauth2.server.resource.authentication.JwtAuthenticationConverter; import org.springframework.security.web.SecurityFilterChain; @@ -113,6 +114,12 @@ public SecurityFilterChain securityFilterChain(@Nonnull final HttpSecurity http) .authenticated()) // Enable CORS as per the configuration. .cors(cors -> cors.configurationSource(corsConfigurationSource())) + // This is a stateless bearer-token API: every request carries its own credentials, so + // no HTTP session or session cookie is ever needed. This also prevents the security + // layer from attempting to save unauthenticated requests to a session during 401 + // handling. + .sessionManagement( + session -> session.sessionCreationPolicy(SessionCreationPolicy.STATELESS)) // Use the provided JWT decoder and authentication converter. .oauth2ResourceServer( oauth2 -> diff --git a/server/src/main/java/au/csiro/pathling/config/SqlQueryConfiguration.java b/server/src/main/java/au/csiro/pathling/config/SqlQueryConfiguration.java index 9d0960bca4..5adeb30436 100644 --- a/server/src/main/java/au/csiro/pathling/config/SqlQueryConfiguration.java +++ b/server/src/main/java/au/csiro/pathling/config/SqlQueryConfiguration.java @@ -22,9 +22,8 @@ import lombok.ToString; /** - * Configuration for the {@code $sqlquery-run} operation. Controls the resource limits applied to - * each query in order to prevent a single request from consuming an unbounded share of server - * resources. + * Configuration for the SQL query operations. Bounds the resolution of a query's dependency graph, + * which happens before any query execution. * * @author John Grimes */ @@ -33,20 +32,12 @@ public class SqlQueryConfiguration { /** - * The maximum number of rows that a single {@code $sqlquery-run} response may stream. Always - * applied; clamps the caller-supplied {@code _limit} when that value is larger. Modelled as a - * {@code long} so that operators can express "effectively disabled" with a value above {@link - * Integer#MAX_VALUE}; the executor clamps to {@code Integer.MAX_VALUE} on the way into Spark's - * {@code limit(int)} API. + * The maximum nesting depth of the dependency graph resolved for a single query. The top-level + * query's direct {@code relatedArtifact} dependencies sit at depth one; each further level of + * nested {@code SQLView} dependency increments the depth. A graph that nests deeper than this + * limit is rejected before any Spark work, guarding against accidental fan-out and runaway + * resolution. Real view graphs are shallow, so the default is generous while still bounded. */ @Min(1) - private long maxRows = 1_000_000L; - - /** - * The maximum wall-clock time in seconds that a single {@code $sqlquery-run} query may run before - * its Spark job group is cancelled. Set to cover the synchronous use case; long-running queries - * should use the asynchronous path. - */ - @Min(1) - private long timeoutSeconds = 60L; + private int maxDependencyDepth = 10; } diff --git a/server/src/main/java/au/csiro/pathling/config/WebConfiguration.java b/server/src/main/java/au/csiro/pathling/config/WebConfiguration.java index f73959c9f1..ef27ad8bc1 100644 --- a/server/src/main/java/au/csiro/pathling/config/WebConfiguration.java +++ b/server/src/main/java/au/csiro/pathling/config/WebConfiguration.java @@ -53,19 +53,31 @@ public void addViewControllers(@Nonnull final ViewControllerRegistry registry) { @Override public void addResourceHandlers(@Nonnull final ResourceHandlerRegistry registry) { - // Serve hashed assets with long cache duration (1 year). These files have content hashes in - // their filenames, so they can be cached indefinitely. + // Serve hashed assets with long cache duration (1 year). The content hash in each filename + // means that a given URL's body can never change, so these files can be cached indefinitely + // and revalidation is never necessary. Marking them immutable stops the browser revalidating + // them even on a user-initiated reload. registry .addResourceHandler(ADMIN_ASSETS_PATH) .addResourceLocations(ADMIN_ASSETS_LOCATION) - .setCacheControl(CacheControl.maxAge(365, TimeUnit.DAYS).cachePublic()); + .setCacheControl(CacheControl.maxAge(365, TimeUnit.DAYS).cachePublic().immutable()); - // Serve admin UI static resources with SPA fallback to index.html. No caching is applied so - // users always get the latest version, which references the current hashed assets. + // Serve admin UI static resources with SPA fallback to index.html. The entry document names + // the hashed assets of one specific build, so a stored copy pins the whole UI to the version + // it was built from and must not be stored at all. + // + // The validator is disabled deliberately. Jib stamps a fixed timestamp into the image for + // reproducibility, so every published image reported the same Last-Modified value. Browsers + // revalidating against it were answered with 304 and kept their old document indefinitely, + // which is what pinned upgraded servers to old bundles. Emitting no validator is also what + // lets an already-stuck client recover, because a handler that still emits one will match the + // client's conditional request whatever the directives say. Do not re-enable this. See + // https://github.com/aehrc/pathling/issues/2677. registry .addResourceHandler(ADMIN_UI_PATHS) .addResourceLocations(ADMIN_UI_RESOURCE_LOCATION) - .setCacheControl(CacheControl.noCache()) + .setCacheControl(CacheControl.noStore().mustRevalidate()) + .setUseLastModified(false) .resourceChain(true) .addResolver( new PathResourceResolver() { diff --git a/server/src/main/java/au/csiro/pathling/errors/ErrorHandlingInterceptor.java b/server/src/main/java/au/csiro/pathling/errors/ErrorHandlingInterceptor.java index 871ea6c0a7..6041a96d06 100644 --- a/server/src/main/java/au/csiro/pathling/errors/ErrorHandlingInterceptor.java +++ b/server/src/main/java/au/csiro/pathling/errors/ErrorHandlingInterceptor.java @@ -17,8 +17,10 @@ package au.csiro.pathling.errors; +import static java.util.Objects.requireNonNull; import static org.springframework.http.HttpStatus.SERVICE_UNAVAILABLE; +import au.csiro.pathling.io.SchemaDriftError; import ca.uhn.fhir.interceptor.api.Hook; import ca.uhn.fhir.interceptor.api.Interceptor; import ca.uhn.fhir.interceptor.api.Pointcut; @@ -135,6 +137,13 @@ public static BaseServerResponseException convertError(@Nonnull final Throwable return new InvalidRequestException(e); } catch (final AccessDeniedError e) { return buildException(HttpServletResponse.SC_FORBIDDEN, e.getMessage(), IssueType.FORBIDDEN); + } catch (final SchemaDriftError e) { + // A drifted, unmigrated table is a server-side deployment state; surface the actionable + // message instead of the generic "Unexpected error occurred". + return buildException( + HttpServletResponse.SC_INTERNAL_SERVER_ERROR, + requireNonNull(e.getMessage()), + IssueType.PROCESSING); } catch (final SparkRuntimeException e) { // SparkRuntimeException with USER_RAISED_EXCEPTION indicates an intentionally raised // error (via raise_error() in Spark SQL) that should be surfaced to the client. @@ -154,7 +163,6 @@ public static BaseServerResponseException convertError(@Nonnull final Throwable } @Nonnull - @SuppressWarnings("SameParameterValue") private static BaseServerResponseException buildException( final int theStatusCode, @Nonnull final String message, @Nonnull final IssueType issueType) { final OperationOutcome opOutcome = new OperationOutcome(); diff --git a/server/src/main/java/au/csiro/pathling/fhir/ConformanceProvider.java b/server/src/main/java/au/csiro/pathling/fhir/ConformanceProvider.java index 0216fdb3b1..9b0d7dc588 100644 --- a/server/src/main/java/au/csiro/pathling/fhir/ConformanceProvider.java +++ b/server/src/main/java/au/csiro/pathling/fhir/ConformanceProvider.java @@ -104,17 +104,46 @@ public class ConformanceProvider private static final String EXPORT_OPERATION = "export"; private static final String RUN_OPERATION = "run"; + private static final String VIEWDEFINITION_RUN_OPERATION = "viewdefinition-run"; + private static final String VIEWDEFINITION_EXPORT_OPERATION = "viewdefinition-export"; + private static final String SQLQUERY_RUN_OPERATION = "sqlquery-run"; + private static final String SQLQUERY_EXPORT_OPERATION = "sqlquery-export"; - /** Base system-level operations available within Pathling. */ + /** + * The spec canonical OperationDefinition URLs for the SQL on FHIR operations. The server declares + * these in the CapabilityStatement instead of Pathling-authored OperationDefinitions, and no + * longer serves a private OperationDefinition for these operations. + */ + private static final String SOF_VIEWDEFINITION_RUN_CANONICAL = + "http://sql-on-fhir.org/OperationDefinition/$viewdefinition-run"; + + private static final String SOF_VIEWDEFINITION_EXPORT_CANONICAL = + "http://sql-on-fhir.org/OperationDefinition/$viewdefinition-export"; + + private static final String SOF_SQLQUERY_RUN_CANONICAL = + "http://sql-on-fhir.org/OperationDefinition/$sqlquery-run"; + + private static final String SOF_SQLQUERY_EXPORT_CANONICAL = + "http://sql-on-fhir.org/OperationDefinition/$sqlquery-export"; + + /** + * Both export operations are parameterised by {@link + * au.csiro.pathling.operations.view.ViewExportFormat}, which supports a narrower set of output + * formats than the spec canonical they declare. Stating the supported set here lets a client + * reading the CapabilityStatement discover the constraint, rather than discovering it as a 400. + */ + private static final String EXPORT_FORMAT_DOCUMENTATION = + "Supported `_format` values: `ndjson` (the default), `csv` and `parquet`. The `json` and" + + " `fhir` formats are not supported for export; a request for either is rejected with a" + + " 400."; + + /** + * Base system-level operations whose Pathling-authored OperationDefinition resources are served. + * The SQL on FHIR run/export operations are intentionally excluded: they declare the spec + * canonical and Pathling does not serve an OperationDefinition for them. + */ private static final List BASE_SYSTEM_OPERATIONS = - Arrays.asList( - "job", - "result", - EXPORT_OPERATION, - "import", - "import-pnp", - "viewdefinition-run", - "viewdefinition-export"); + Arrays.asList("job", "jobs", "result", EXPORT_OPERATION, "import", "import-pnp"); /** Bulk submit operations, added when bulk submit is configured. */ private static final List BULK_SUBMIT_OPERATIONS = @@ -129,9 +158,12 @@ public class ConformanceProvider private static final String FHIR_RESOURCE_BASE = "http://hl7.org/fhir/StructureDefinition/"; private static final String UNKNOWN_VERSION = "UNKNOWN"; - /** All resource-level operations available within Pathling. */ - private static final List RESOURCE_LEVEL_OPERATIONS = - List.of(EXPORT_OPERATION, RUN_OPERATION); + /** + * Resource-level operations whose Pathling-authored OperationDefinition resources are served. The + * ViewDefinition {@code $run} operation is excluded: it declares the spec canonical and Pathling + * does not serve an OperationDefinition for it. + */ + private static final List RESOURCE_LEVEL_OPERATIONS = List.of(EXPORT_OPERATION); /** Resource types that have the export operation available. */ private static final Set EXPORT_RESOURCE_TYPES = @@ -468,9 +500,9 @@ private List buildResources() { viewDefResource.addInteraction(viewDefDeleteInteraction); } - // Add $run operation to ViewDefinition resource if enabled. + // Add $run operation to ViewDefinition resource if enabled, declaring the spec canonical. if (ops.isViewDefinitionInstanceRunEnabled()) { - final CanonicalType runUri = new CanonicalType(getOperationUri(RUN_OPERATION)); + final CanonicalType runUri = new CanonicalType(SOF_VIEWDEFINITION_RUN_CANONICAL); final CapabilityStatementRestResourceOperationComponent runOp = new CapabilityStatementRestResourceOperationComponent( new StringType(RUN_OPERATION), runUri); @@ -487,8 +519,9 @@ private List buildOperations( final List operations = new ArrayList<>(); final OperationConfiguration ops = configuration.getOperations(); - // Add job operation (always included when async is enabled). + // Add job operations (always included when async is enabled). addOperationIfEnabled(operations, "job", true); + addOperationIfEnabled(operations, "jobs", true); // Add result operation (needed for export results). addOperationIfEnabled(operations, "result", ops.isAnyExportEnabled()); @@ -500,12 +533,31 @@ private List buildOperations( addOperationIfEnabled(operations, "import", ops.isImportEnabled()); addOperationIfEnabled(operations, "import-pnp", ops.isImportPnpEnabled()); - // Add viewdefinition operations. - addOperationIfEnabled(operations, "viewdefinition-run", ops.isViewDefinitionRunEnabled()); - addOperationIfEnabled(operations, "viewdefinition-export", ops.isViewDefinitionExportEnabled()); - - // Add SQL query run operation. - addOperationIfEnabled(operations, "sqlquery-run", ops.isSqlQueryRunEnabled()); + // Add viewdefinition operations, declaring the SQL on FHIR spec canonicals. + addOperationIfEnabled( + operations, + VIEWDEFINITION_RUN_OPERATION, + ops.isViewDefinitionRunEnabled(), + SOF_VIEWDEFINITION_RUN_CANONICAL); + addOperationIfEnabled( + operations, + VIEWDEFINITION_EXPORT_OPERATION, + ops.isViewDefinitionExportEnabled(), + SOF_VIEWDEFINITION_EXPORT_CANONICAL, + EXPORT_FORMAT_DOCUMENTATION); + + // Add SQL query run operation, declaring the SQL on FHIR spec canonical. + addOperationIfEnabled( + operations, SQLQUERY_RUN_OPERATION, ops.isSqlQueryRunEnabled(), SOF_SQLQUERY_RUN_CANONICAL); + + // Add SQL query export operation, declaring the SQL on FHIR spec canonical. The referenced + // OperationDefinition declares the system, type, and instance scopes. + addOperationIfEnabled( + operations, + SQLQUERY_EXPORT_OPERATION, + ops.isSqlQueryExportEnabled(), + SOF_SQLQUERY_EXPORT_CANONICAL, + EXPORT_FORMAT_DOCUMENTATION); // Add bulk submit operations if configured and enabled. if (configuration.getBulkSubmit() != null && ops.isBulkSubmitEnabled()) { @@ -520,11 +572,31 @@ private void addOperationIfEnabled( final List operations, final String name, final boolean enabled) { + addOperationIfEnabled(operations, name, enabled, getOperationUri(name)); + } + + private void addOperationIfEnabled( + final List operations, + final String name, + final boolean enabled, + final String definitionUri) { + addOperationIfEnabled(operations, name, enabled, definitionUri, null); + } + + private void addOperationIfEnabled( + final List operations, + final String name, + final boolean enabled, + final String definitionUri, + @Nullable final String documentation) { if (enabled) { - final CanonicalType operationUri = new CanonicalType(getOperationUri(name)); - operations.add( + final CapabilityStatementRestResourceOperationComponent operation = new CapabilityStatementRestResourceOperationComponent( - new StringType(name), operationUri)); + new StringType(name), new CanonicalType(definitionUri)); + if (documentation != null) { + operation.setDocumentation(documentation); + } + operations.add(operation); } } diff --git a/server/src/main/java/au/csiro/pathling/io/DriftGuardedSource.java b/server/src/main/java/au/csiro/pathling/io/DriftGuardedSource.java new file mode 100644 index 0000000000..0de91debcd --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/io/DriftGuardedSource.java @@ -0,0 +1,137 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.io; + +import au.csiro.pathling.io.source.DataSource; +import au.csiro.pathling.library.io.sink.DataSinkBuilder; +import au.csiro.pathling.library.io.source.QueryableDataSource; +import au.csiro.pathling.library.query.FhirViewQuery; +import au.csiro.pathling.views.FhirView; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.util.Set; +import java.util.function.BiFunction; +import java.util.function.Predicate; +import org.apache.spark.sql.Dataset; +import org.apache.spark.sql.Row; + +/** + * A {@link QueryableDataSource} wrapper that guards reads against schema drift, and carries that + * guard into sources derived from it through {@code map} or {@code filterByResourceType}. Reads of + * a drifted type fail with {@link SchemaDriftError} instead of an opaque Spark analysis failure. + * View queries are guarded on their subject resource type when the query is constructed, because + * the executed query resolves datasets through the wrapped source's own dispatcher rather than the + * guarded {@code read}. + * + *

The drifted types set is held by reference, so guard decisions reflect its current contents. + * This allows a mutable set shared with a refreshing source to clear the guard when a type is + * successfully migrated. + * + * @author John Grimes + */ +public class DriftGuardedSource implements QueryableDataSource { + + /** The underlying QueryableDataSource that guarded operations delegate to. */ + @Nonnull protected final QueryableDataSource delegate; + + /** The resource types whose tables are drifted and unmigrated. */ + @Nonnull protected final Set driftedTypes; + + /** + * Constructs a new DriftGuardedSource. + * + * @param delegate the underlying QueryableDataSource to delegate to + * @param driftedTypes the resource types whose tables are drifted and unmigrated; held by + * reference so that mutations are reflected in guard decisions + */ + public DriftGuardedSource( + @Nonnull final QueryableDataSource delegate, @Nonnull final Set driftedTypes) { + this.delegate = delegate; + this.driftedTypes = driftedTypes; + } + + @Override + @Nonnull + public Dataset read(@Nullable final String resourceCode) { + checkNotDrifted(resourceCode); + return delegate.read(resourceCode); + } + + @Override + @Nonnull + public Set getResourceTypes() { + return delegate.getResourceTypes(); + } + + @Override + @Nonnull + public DataSinkBuilder write() { + return delegate.write(); + } + + @Override + @Nonnull + public FhirViewQuery view(@Nullable final String subjectResource) { + checkNotDrifted(subjectResource); + return delegate.view(subjectResource); + } + + @Override + @Nonnull + public FhirViewQuery view(@Nullable final FhirView view) { + if (view != null) { + checkNotDrifted(view.getResource()); + } + return delegate.view(view); + } + + @Override + @Nonnull + public QueryableDataSource map( + @Nonnull final BiFunction, Dataset> operator) { + return new DriftGuardedSource(delegate.map(operator), driftedTypes); + } + + @Override + @Nonnull + public QueryableDataSource filterByResourceType( + @Nonnull final Predicate resourceTypePredicate) { + return new DriftGuardedSource( + delegate.filterByResourceType(resourceTypePredicate), driftedTypes); + } + + @Override + @Nonnull + public DataSource cache() { + final DataSource cached = delegate.cache(); + return cached instanceof final QueryableDataSource queryable + ? new DriftGuardedSource(queryable, driftedTypes) + : cached; + } + + /** + * Fails with a {@link SchemaDriftError} if the given resource type is marked as drifted. + * + * @param resourceCode the resource type code to check, or null to skip the check + */ + protected final void checkNotDrifted(@Nullable final String resourceCode) { + if (resourceCode != null && driftedTypes.contains(resourceCode)) { + throw new SchemaDriftError(resourceCode); + } + } +} diff --git a/server/src/main/java/au/csiro/pathling/io/DynamicDeltaSource.java b/server/src/main/java/au/csiro/pathling/io/DynamicDeltaSource.java index 6d2ec2eafe..eb41d7ddd8 100644 --- a/server/src/main/java/au/csiro/pathling/io/DynamicDeltaSource.java +++ b/server/src/main/java/au/csiro/pathling/io/DynamicDeltaSource.java @@ -20,36 +20,33 @@ import au.csiro.pathling.QueryHelpers; import au.csiro.pathling.config.StorageConfiguration; import au.csiro.pathling.encoders.FhirEncoders; -import au.csiro.pathling.io.source.DataSource; import au.csiro.pathling.library.io.FileSystemPersistence; -import au.csiro.pathling.library.io.sink.DataSinkBuilder; +import au.csiro.pathling.library.io.source.DatasetSource; import au.csiro.pathling.library.io.source.QueryableDataSource; -import au.csiro.pathling.library.query.FhirViewQuery; -import au.csiro.pathling.views.FhirView; import io.delta.tables.DeltaTable; import jakarta.annotation.Nonnull; import jakarta.annotation.Nullable; import java.util.HashSet; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.function.BiFunction; -import java.util.function.Predicate; import lombok.extern.slf4j.Slf4j; import org.apache.spark.sql.Dataset; import org.apache.spark.sql.Row; import org.apache.spark.sql.SparkSession; /** - * A QueryableDataSource wrapper that dynamically discovers new resource types created after - * startup. Delegates to the underlying data source for known types, and attempts on-demand - * discovery for unknown types by checking if a Delta table exists at the expected path. + * A {@link DriftGuardedSource} that dynamically discovers new resource types created after startup. + * Delegates to the underlying data source for known types, and attempts on-demand discovery for + * unknown types by checking if a Delta table exists at the expected path. + * + *

The drift guard behaviour, including its propagation into derived sources, is inherited from + * {@link DriftGuardedSource}. The drifted types set is mutable so that a successful {@link + * #refresh} clears the guard for the refreshed type. * * @author John Grimes */ @Slf4j -public class DynamicDeltaSource implements QueryableDataSource { - - @Nonnull private final QueryableDataSource delegate; +public class DynamicDeltaSource extends DriftGuardedSource { @Nonnull private final SparkSession spark; @@ -62,7 +59,7 @@ public class DynamicDeltaSource implements QueryableDataSource { @Nonnull private final Set dynamicallyDiscoveredTypes = ConcurrentHashMap.newKeySet(); /** - * Constructs a new DynamicDeltaSource. + * Constructs a new DynamicDeltaSource with no drifted types. * * @param delegate the underlying QueryableDataSource to delegate to * @param spark the Spark session for Delta table operations @@ -76,13 +73,47 @@ public DynamicDeltaSource( @Nonnull final String databasePath, @Nonnull final FhirEncoders fhirEncoders, @Nonnull final StorageConfiguration storageConfiguration) { - this.delegate = delegate; + this(delegate, spark, databasePath, fhirEncoders, storageConfiguration, Set.of()); + } + + /** + * Constructs a new DynamicDeltaSource. + * + * @param delegate the underlying QueryableDataSource to delegate to + * @param spark the Spark session for Delta table operations + * @param databasePath the path to the Delta database + * @param fhirEncoders the FHIR encoders for creating empty datasets + * @param storageConfiguration the storage configuration + * @param driftedTypes the resource types left drifted and unmigrated at startup + */ + public DynamicDeltaSource( + @Nonnull final QueryableDataSource delegate, + @Nonnull final SparkSession spark, + @Nonnull final String databasePath, + @Nonnull final FhirEncoders fhirEncoders, + @Nonnull final StorageConfiguration storageConfiguration, + @Nonnull final Set driftedTypes) { + super(delegate, concurrentCopyOf(driftedTypes)); this.spark = spark; this.databasePath = databasePath; this.fhirEncoders = fhirEncoders; this.cacheDatasets = storageConfiguration.getCacheDatasets(); } + /** + * Copies the given types into a mutable concurrent set, so that the drifted mark can be cleared + * by {@link #refresh} and observed by derived sources. + * + * @param types the types to copy + * @return a mutable concurrent set containing the given types + */ + @Nonnull + private static Set concurrentCopyOf(@Nonnull final Set types) { + final Set copy = ConcurrentHashMap.newKeySet(); + copy.addAll(types); + return copy; + } + @Override @Nonnull public Dataset read(@Nullable final String resourceCode) { @@ -90,6 +121,10 @@ public Dataset read(@Nullable final String resourceCode) { throw new IllegalArgumentException("Resource code must not be null"); } + // A type whose table is drifted and unmigrated cannot be queried; fail with an actionable + // error rather than an opaque analysis failure. + checkNotDrifted(resourceCode); + // If delegate knows about this type, use it. if (delegate.getResourceTypes().contains(resourceCode)) { return cacheIfEnabled(delegate.read(resourceCode)); @@ -113,50 +148,52 @@ public Dataset read(@Nullable final String resourceCode) { return QueryHelpers.createEmptyDataset(spark, fhirEncoders, resourceCode); } - @Override - @Nonnull - public Set getResourceTypes() { - final Set types = new HashSet<>(delegate.getResourceTypes()); - types.addAll(dynamicallyDiscoveredTypes); - return types; - } - - @Override - @Nonnull - public DataSinkBuilder write() { - return delegate.write(); - } - - @Override - @Nonnull - public FhirViewQuery view(@Nullable final String subjectResource) { - return delegate.view(subjectResource); - } + /** + * Re-loads the Delta table for the given resource type and replaces the dataset served for it, so + * that all consumers observe the table's current schema. Intended to be called after a + * schema-evolving write. When dataset caching is enabled, the stale cached dataset is + * unpersisted. If no Delta table exists for the type, the call is a no-op. + * + * @param resourceCode the resource type code to refresh + */ + public void refresh(@Nonnull final String resourceCode) { + final String tablePath = getTablePath(resourceCode); + if (!DeltaTable.isDeltaTable(spark, tablePath)) { + log.debug("No Delta table found for resource type {}, nothing to refresh", resourceCode); + return; + } - @Override - @Nonnull - public FhirViewQuery view(@Nullable final FhirView view) { - return delegate.view(view); - } + // Unpersist the stale cached dataset before replacing it, so the cached plan for the old + // snapshot does not linger in the Spark cache. + if (cacheDatasets && delegate.getResourceTypes().contains(resourceCode)) { + delegate.read(resourceCode).unpersist(); + } - @Override - @Nonnull - public QueryableDataSource map( - @Nonnull final BiFunction, Dataset> operator) { - return delegate.map(operator); - } + final Dataset refreshed = spark.read().format("delta").load(tablePath); + if (delegate instanceof final DatasetSource datasetSource) { + // Replace the pinned entry in the delegate's resource map, so every consumer that resolves + // datasets through the delegate observes the evolved schema. + datasetSource.dataset(resourceCode, refreshed); + log.info("Refreshed dataset for resource type {}", resourceCode); + } else { + // The delegate cannot be mutated; serve the type through dynamic discovery, which re-loads + // the Delta table on each read. + dynamicallyDiscoveredTypes.add(resourceCode); + log.info("Registered resource type {} for dynamic discovery following refresh", resourceCode); + } - @Override - @Nonnull - public QueryableDataSource filterByResourceType( - @Nonnull final Predicate resourceTypePredicate) { - return delegate.filterByResourceType(resourceTypePredicate); + // The freshly loaded table carries the current schema, so the type is no longer drifted. + if (driftedTypes.remove(resourceCode)) { + log.info("Cleared drifted mark for resource type {}", resourceCode); + } } @Override @Nonnull - public DataSource cache() { - return delegate.cache(); + public Set getResourceTypes() { + final Set types = new HashSet<>(delegate.getResourceTypes()); + types.addAll(dynamicallyDiscoveredTypes); + return types; } @Nonnull diff --git a/server/src/main/java/au/csiro/pathling/io/JobDirectoryFileSystem.java b/server/src/main/java/au/csiro/pathling/io/JobDirectoryFileSystem.java index 5918f995a9..2585f0ccd5 100644 --- a/server/src/main/java/au/csiro/pathling/io/JobDirectoryFileSystem.java +++ b/server/src/main/java/au/csiro/pathling/io/JobDirectoryFileSystem.java @@ -145,6 +145,25 @@ public void ensureJobDirectory(@Nonnull final String jobId) throws IOException { } } + /** + * Recursively deletes the per-job directory and all its contents. Deleting a directory that does + * not exist is a no-op. + * + *

This is a strict primitive: any I/O failure is thrown. Callers that want best-effort cleanup + * (for example, removing a partial output directory after an export fails) catch and log. + * + * @param jobId the job identifier + * @throws IllegalArgumentException if the job identifier contains traversal sequences + * @throws IOException if the directory exists but could not be deleted + */ + public void deleteJobDirectory(@Nonnull final String jobId) throws IOException { + final FileSystem fs = getFileSystem(); + final Path jobDir = jobDirectory(jobId); + if (fs.exists(jobDir) && !fs.delete(jobDir, /* recursive= */ true)) { + throw new IOException("Failed to delete job directory: " + jobDir); + } + } + /** * Resolves a file for reading, validating containment, existence and (for {@code file://} * schemes) symlink-aware containment, then opens an input stream. diff --git a/server/src/main/java/au/csiro/pathling/io/SchemaDrift.java b/server/src/main/java/au/csiro/pathling/io/SchemaDrift.java new file mode 100644 index 0000000000..f7de9e4f62 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/io/SchemaDrift.java @@ -0,0 +1,93 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.io; + +import jakarta.annotation.Nonnull; +import java.util.HashSet; +import java.util.Set; +import java.util.SortedSet; +import java.util.TreeSet; +import org.apache.spark.sql.types.ArrayType; +import org.apache.spark.sql.types.DataType; +import org.apache.spark.sql.types.MapType; +import org.apache.spark.sql.types.StructField; +import org.apache.spark.sql.types.StructType; + +/** + * Detects schema drift between the schema produced by the current FHIR encoders and the schema of + * an existing Delta table. Only field paths present in the source but absent from the target count + * as drift; nullability and column metadata differences are ignored, and extra target-only fields + * are tolerated. + * + * @author John Grimes + */ +public final class SchemaDrift { + + private SchemaDrift() {} + + /** + * Returns true if the source schema contains any field path (recursing through structs, arrays + * and maps) that is absent from the target schema. + * + * @param source the candidate schema (typically the encoder output) + * @param target the existing table schema + * @return true if {@code source} introduces at least one field name not present in {@code target} + */ + public static boolean hasMissingFields( + @Nonnull final StructType source, @Nonnull final StructType target) { + return !missingFieldPaths(source, target).isEmpty(); + } + + /** + * Returns the field paths present in the source schema but absent from the target schema, in + * lexicographic order. + * + * @param source the candidate schema (typically the encoder output) + * @param target the existing table schema + * @return the missing field paths, dot-separated + */ + @Nonnull + public static SortedSet missingFieldPaths( + @Nonnull final StructType source, @Nonnull final StructType target) { + final SortedSet missing = new TreeSet<>(collectFieldPaths(source)); + missing.removeAll(collectFieldPaths(target)); + return missing; + } + + @Nonnull + private static Set collectFieldPaths(@Nonnull final StructType schema) { + final Set paths = new HashSet<>(); + collectFieldPaths(schema, "", paths); + return paths; + } + + private static void collectFieldPaths( + @Nonnull final DataType type, @Nonnull final String prefix, @Nonnull final Set out) { + if (type instanceof final StructType struct) { + for (final StructField field : struct.fields()) { + final String path = prefix.isEmpty() ? field.name() : prefix + "." + field.name(); + out.add(path); + collectFieldPaths(field.dataType(), path, out); + } + } else if (type instanceof final ArrayType array) { + collectFieldPaths(array.elementType(), prefix, out); + } else if (type instanceof final MapType map) { + collectFieldPaths(map.valueType(), prefix, out); + } + } +} diff --git a/server/src/main/java/au/csiro/pathling/io/SchemaDriftError.java b/server/src/main/java/au/csiro/pathling/io/SchemaDriftError.java new file mode 100644 index 0000000000..3e788b79fb --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/io/SchemaDriftError.java @@ -0,0 +1,60 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.io; + +import jakarta.annotation.Nonnull; + +/** + * Raised when a request requires the data of a resource type whose Delta table schema is behind + * this server's encoders and has not been migrated. The message names the affected type, the + * condition, and the available remedies, and is surfaced to API clients through an + * OperationOutcome. + * + * @author John Grimes + */ +public class SchemaDriftError extends RuntimeException { + + private static final long serialVersionUID = 1L; + + @Nonnull private final String resourceCode; + + /** + * Constructs a new SchemaDriftError for the given resource type. + * + * @param resourceCode the resource type whose table is drifted and unmigrated + */ + public SchemaDriftError(@Nonnull final String resourceCode) { + super( + "The stored table for resource type '" + + resourceCode + + "' has a schema that is behind this server's encoders and cannot be queried. " + + "Enable pathling.storage.schemaAutoMerge (or restore write access to the " + + "warehouse) and restart, or update a resource of this type, to migrate the table."); + this.resourceCode = resourceCode; + } + + /** + * Returns the resource type whose table is drifted. + * + * @return the resource type code + */ + @Nonnull + public String getResourceCode() { + return resourceCode; + } +} diff --git a/server/src/main/java/au/csiro/pathling/io/SchemaMigrator.java b/server/src/main/java/au/csiro/pathling/io/SchemaMigrator.java new file mode 100644 index 0000000000..a33feeae1f --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/io/SchemaMigrator.java @@ -0,0 +1,183 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.io; + +import static au.csiro.pathling.library.io.FileSystemPersistence.safelyJoinPaths; + +import au.csiro.pathling.QueryHelpers; +import au.csiro.pathling.encoders.FhirEncoders; +import io.delta.tables.DeltaTable; +import jakarta.annotation.Nonnull; +import java.io.IOException; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.SortedSet; +import lombok.extern.slf4j.Slf4j; +import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.spark.sql.SaveMode; +import org.apache.spark.sql.SparkSession; +import org.apache.spark.sql.types.StructType; + +/** + * Detects Delta tables in the warehouse whose schemas are missing fields relative to the current + * FHIR encoders, and migrates them at startup when {@code schemaAutoMerge} is enabled. Tables that + * remain drifted (flag disabled, or migration failure) are reported so that requests against them + * can fail with an actionable error instead of a generic one. + * + * @author John Grimes + */ +@Slf4j +public class SchemaMigrator { + + /** The file extension used for Delta table directories within the warehouse. */ + private static final String TABLE_EXTENSION = ".parquet"; + + @Nonnull private final SparkSession spark; + + @Nonnull private final FhirEncoders fhirEncoders; + + @Nonnull private final String databasePath; + + private final boolean schemaAutoMerge; + + /** + * Constructs a new SchemaMigrator. + * + * @param spark the Spark session for Delta table operations + * @param fhirEncoders the FHIR encoders whose schemas are the migration target + * @param databasePath the path to the Delta database + * @param schemaAutoMerge whether schema migration is enabled + */ + public SchemaMigrator( + @Nonnull final SparkSession spark, + @Nonnull final FhirEncoders fhirEncoders, + @Nonnull final String databasePath, + final boolean schemaAutoMerge) { + this.spark = spark; + this.fhirEncoders = fhirEncoders; + this.databasePath = databasePath; + this.schemaAutoMerge = schemaAutoMerge; + } + + /** + * Compares every Delta table in the database path against the current encoder schema for its + * resource type, migrating drifted tables when {@code schemaAutoMerge} is enabled. Migration is + * additive only and never fails startup: per-table failures are logged and reported through the + * returned set. + * + * @return the resource type codes that remain drifted and unmigrated + */ + @Nonnull + public Set migrate() { + final Set driftedTypes = new HashSet<>(); + for (final String resourceCode : listTableResourceCodes()) { + checkTable(resourceCode, driftedTypes); + } + return driftedTypes; + } + + /** + * Compares one table against its encoder schema and migrates it if drifted and permitted, + * recording the type in the drifted set when it remains unmigrated. Never throws (FR-006). + */ + private void checkTable( + @Nonnull final String resourceCode, @Nonnull final Set driftedTypes) { + final String tablePath = safelyJoinPaths(databasePath, resourceCode + TABLE_EXTENSION); + final SortedSet missingFields; + try { + if (!DeltaTable.isDeltaTable(spark, tablePath)) { + return; + } + final StructType encoderSchema = fhirEncoders.of(resourceCode).schema(); + final StructType tableSchema = spark.read().format("delta").load(tablePath).schema(); + missingFields = SchemaDrift.missingFieldPaths(encoderSchema, tableSchema); + } catch (final Exception e) { + // A table that cannot be inspected (for example, an unencodable name) is skipped. + log.debug("Skipping schema drift check for {}: {}", resourceCode, e.getMessage()); + return; + } + + if (missingFields.isEmpty()) { + return; + } + + if (!schemaAutoMerge) { + log.warn( + "The {} table schema is behind this server's encoders (missing fields: {}). Requests " + + "against this type will fail until it is migrated. Enable " + + "pathling.storage.schemaAutoMerge and restart, or update a resource of this type " + + "with the flag enabled, to migrate the table.", + resourceCode, + missingFields); + driftedTypes.add(resourceCode); + return; + } + + try { + // A zero-row append with mergeSchema adds the missing fields (including fields nested + // inside structs, arrays and maps) to the table schema without touching any data; existing + // rows present the new fields as null. + QueryHelpers.createEmptyDataset(spark, fhirEncoders, resourceCode) + .write() + .format("delta") + .mode(SaveMode.Append) + .option("mergeSchema", "true") + .save(tablePath); + log.info("Migrated schema of {} table, added fields: {}", resourceCode, missingFields); + } catch (final Exception e) { + log.error( + "Failed to migrate the schema of the {} table (missing fields: {}). Requests against " + + "this type will fail until it is migrated.", + resourceCode, + missingFields, + e); + driftedTypes.add(resourceCode); + } + } + + /** + * Lists the resource type codes for which a Delta table directory exists in the database path. + * Non-directories and entries without the expected extension are skipped; a missing or empty + * database path yields an empty list. + */ + @Nonnull + private List listTableResourceCodes() { + final List resourceCodes = new ArrayList<>(); + try { + final Path dbPath = new Path(databasePath); + final FileSystem fileSystem = + dbPath.getFileSystem(spark.sparkContext().hadoopConfiguration()); + if (!fileSystem.exists(dbPath)) { + return resourceCodes; + } + for (final FileStatus status : fileSystem.listStatus(dbPath)) { + final String name = status.getPath().getName(); + if (status.isDirectory() && name.endsWith(TABLE_EXTENSION)) { + resourceCodes.add(name.substring(0, name.length() - TABLE_EXTENSION.length())); + } + } + } catch (final IOException e) { + log.warn("Unable to scan the warehouse for schema drift: {}", e.getMessage()); + } + return resourceCodes; + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/ParquetSchemaValidator.java b/server/src/main/java/au/csiro/pathling/operations/ParquetSchemaValidator.java new file mode 100644 index 0000000000..7220f6de0a --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/ParquetSchemaValidator.java @@ -0,0 +1,99 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations; + +import au.csiro.pathling.errors.InvalidUserInputError; +import jakarta.annotation.Nonnull; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Collectors; +import org.apache.spark.sql.types.ArrayType; +import org.apache.spark.sql.types.DataType; +import org.apache.spark.sql.types.MapType; +import org.apache.spark.sql.types.NullType; +import org.apache.spark.sql.types.StructField; +import org.apache.spark.sql.types.StructType; + +/** + * Validates that a Spark result schema can be written to Parquet, rejecting schemas that contain an + * unresolved ({@code NullType}, displayed as "VOID") type with a clear, user-correctable error. + * + *

Spark's Parquet writer rejects a {@code NullType} anywhere in the schema, not just at the top + * level (for example, an {@code array()} literal produces an array of {@code NullType}). The walk + * therefore recurses into structs, arrays, and maps and reports every offending field path in a + * single message. + * + * @author John Grimes + */ +public final class ParquetSchemaValidator { + + private ParquetSchemaValidator() { + // Utility class; not instantiable. + } + + /** + * Validates that the given schema contains no unresolved (VOID) types anywhere in its structure. + * + * @param schema the result schema to validate + * @throws InvalidUserInputError if the schema contains one or more {@code NullType} fields; the + * message names every offending field path and suggests both remediations + */ + public static void validateSchemaForParquet(@Nonnull final StructType schema) { + final List voidPaths = new ArrayList<>(); + walkStruct(schema, "", voidPaths); + + if (!voidPaths.isEmpty()) { + final String columns = + voidPaths.stream().map(path -> "'" + path + "'").collect(Collectors.joining(", ")); + throw new InvalidUserInputError( + "The result contains column(s) with an unresolved (VOID) type that cannot be written to " + + "Parquet: " + + columns + + ". Add an explicit CAST to the query or view (e.g. CAST(column AS STRING)), or " + + "choose a different output format."); + } + } + + /** Recurses over the fields of a struct, extending the accumulated field path for each. */ + private static void walkStruct( + @Nonnull final StructType struct, + @Nonnull final String path, + @Nonnull final List voidPaths) { + for (final StructField field : struct.fields()) { + final String fieldPath = path.isEmpty() ? field.name() : path + "." + field.name(); + walkType(field.dataType(), fieldPath, voidPaths); + } + } + + /** Recurses over a single data type, recording the path when a {@code NullType} is reached. */ + private static void walkType( + @Nonnull final DataType dataType, + @Nonnull final String path, + @Nonnull final List voidPaths) { + if (dataType instanceof NullType) { + voidPaths.add(path); + } else if (dataType instanceof final StructType struct) { + walkStruct(struct, path, voidPaths); + } else if (dataType instanceof final ArrayType array) { + walkType(array.elementType(), path + "[]", voidPaths); + } else if (dataType instanceof final MapType map) { + walkType(map.keyType(), path + ".key", voidPaths); + walkType(map.valueType(), path + ".value", voidPaths); + } + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/bulkexport/ExportExecutor.java b/server/src/main/java/au/csiro/pathling/operations/bulkexport/ExportExecutor.java index 91a92f27f7..9931f9462f 100644 --- a/server/src/main/java/au/csiro/pathling/operations/bulkexport/ExportExecutor.java +++ b/server/src/main/java/au/csiro/pathling/operations/bulkexport/ExportExecutor.java @@ -26,6 +26,7 @@ import static org.apache.spark.sql.functions.struct; import au.csiro.pathling.config.ServerConfiguration; +import au.csiro.pathling.io.JobDirectoryFileSystem; import au.csiro.pathling.library.PathlingContext; import au.csiro.pathling.library.io.sink.DataSinkBuilder; import au.csiro.pathling.library.io.sink.WriteDetails; @@ -41,7 +42,6 @@ import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; import jakarta.annotation.Nonnull; import java.io.IOException; -import java.net.URI; import java.util.Arrays; import java.util.HashSet; import java.util.Map; @@ -49,19 +49,15 @@ import java.util.function.UnaryOperator; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; -import org.apache.hadoop.conf.Configuration; -import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.spark.sql.Column; import org.apache.spark.sql.Dataset; import org.apache.spark.sql.Row; -import org.apache.spark.sql.SparkSession; import org.apache.spark.sql.types.DataType; import org.apache.spark.sql.types.DataTypes; import org.apache.spark.sql.types.MapType; import org.apache.spark.sql.types.StructField; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.beans.factory.annotation.Value; import org.springframework.stereotype.Component; /** @@ -80,9 +76,7 @@ public class ExportExecutor { @Nonnull private final FhirContext fhirContext; - @Nonnull private final SparkSession sparkSession; - - @Nonnull private final String databasePath; + @Nonnull private final JobDirectoryFileSystem jobDirectoryFileSystem; @Nonnull private final ServerConfiguration serverConfiguration; @@ -94,8 +88,8 @@ public class ExportExecutor { * @param pathlingContext The Pathling context. * @param deltaLake The queryable data source. * @param fhirContext The FHIR context. - * @param sparkSession The Spark session. - * @param databasePath The database path. + * @param jobDirectoryFileSystem The shared helper that creates per-job directories on the + * warehouse filesystem. * @param serverConfiguration The server configuration. * @param patientCompartmentService The patient compartment service. */ @@ -104,16 +98,13 @@ public ExportExecutor( @Nonnull final PathlingContext pathlingContext, @Nonnull final QueryableDataSource deltaLake, @Nonnull final FhirContext fhirContext, - @Nonnull final SparkSession sparkSession, - @Nonnull @Value("${pathling.storage.warehouseUrl}/${pathling.storage.databaseName}") - final String databasePath, + @Nonnull final JobDirectoryFileSystem jobDirectoryFileSystem, @Nonnull final ServerConfiguration serverConfiguration, @Nonnull final PatientCompartmentService patientCompartmentService) { this.pathlingContext = pathlingContext; this.deltaLake = deltaLake; this.fhirContext = fhirContext; - this.sparkSession = sparkSession; - this.databasePath = databasePath; + this.jobDirectoryFileSystem = jobDirectoryFileSystem; this.serverConfiguration = serverConfiguration; this.patientCompartmentService = patientCompartmentService; } @@ -204,38 +195,30 @@ private ExportResponse writeResultToJobDirectory( @Nonnull final ExportRequest exportRequest, @Nonnull final String jobId, @Nonnull final QueryableDataSource mapped) { - final URI warehouseUri = URI.create(databasePath); - final Path warehousePath = new Path(warehouseUri); - final Path jobDirPath = new Path(new Path(warehousePath, "jobs"), jobId); - final Configuration configuration = sparkSession.sparkContext().hadoopConfiguration(); + final Path jobDirPath; try { - final FileSystem fs = FileSystem.get(configuration); - if (!fs.exists(jobDirPath)) { - final boolean created = fs.mkdirs(jobDirPath); - if (!created) { - throw new InternalErrorException( - "Failed to created subdirectory at %s for job %s.".formatted(databasePath, jobId)); - } - log.debug("Created dir {}", jobDirPath); - } - - final DataSinkBuilder sinkBuilder = - new DataSinkBuilder(pathlingContext, mapped).saveMode("overwrite"); - final WriteDetails writeDetails = - switch (exportRequest.outputFormat()) { - case NDJSON -> sinkBuilder.ndjson(jobDirPath.toString()); - case PARQUET -> sinkBuilder.parquet(jobDirPath.toString()); - case null -> sinkBuilder.ndjson(jobDirPath.toString()); - }; - return new ExportResponse( - exportRequest.originalRequest(), - exportRequest.serverBaseUrl(), - writeDetails, - serverConfiguration.getAuth().isEnabled()); + // The helper resolves the filesystem from the warehouse URI, so directory creation works on + // any warehouse scheme, not just the process default filesystem. + jobDirectoryFileSystem.ensureJobDirectory(jobId); + jobDirPath = jobDirectoryFileSystem.jobDirectory(jobId); } catch (final IOException e) { throw new InternalErrorException( - "Failed to created subdirectory at %s for job %s.".formatted(databasePath, jobId)); + "Failed to create job directory for job %s.".formatted(jobId), e); } + + final DataSinkBuilder sinkBuilder = + new DataSinkBuilder(pathlingContext, mapped).saveMode("overwrite"); + final WriteDetails writeDetails = + switch (exportRequest.outputFormat()) { + case NDJSON -> sinkBuilder.ndjson(jobDirPath.toString()); + case PARQUET -> sinkBuilder.parquet(jobDirPath.toString()); + case null -> sinkBuilder.ndjson(jobDirPath.toString()); + }; + return new ExportResponse( + exportRequest.originalRequest(), + exportRequest.serverBaseUrl(), + writeDetails, + serverConfiguration.getAuth().isEnabled()); } @Nonnull diff --git a/server/src/main/java/au/csiro/pathling/operations/bulkimport/ImportPnpExecutor.java b/server/src/main/java/au/csiro/pathling/operations/bulkimport/ImportPnpExecutor.java index 069f27a970..9f394e31b1 100644 --- a/server/src/main/java/au/csiro/pathling/operations/bulkimport/ImportPnpExecutor.java +++ b/server/src/main/java/au/csiro/pathling/operations/bulkimport/ImportPnpExecutor.java @@ -22,6 +22,7 @@ import au.csiro.fhir.export.BulkExportResult; import au.csiro.fhir.export.BulkExportResult.FileResult; import au.csiro.filestore.hdfs.HdfsFileStoreFactory; +import au.csiro.http.HttpClientConfig; import au.csiro.pathling.config.PnpConfiguration; import au.csiro.pathling.config.ServerConfiguration; import au.csiro.pathling.errors.InvalidUserInputError; @@ -144,7 +145,7 @@ public ImportResponse execute(@Nonnull final ImportPnpRequest pnpRequest, final // Download files using fhir-bulk-java. final Path outputDir = new Path(tempDir, "export-output"); - final BulkExportClient client = buildBulkExportClient(pnpRequest, pnpConfig, outputDir); + final BulkExportClient client = buildBulkExportClient(pnpRequest, pnpConfig, outputDir, fs); final Map> downloadedFiles = downloadFiles(client, pnpRequest.exportUrl(), outputDir, fs, fileExtension); @@ -171,8 +172,11 @@ public ImportResponse execute(@Nonnull final ImportPnpRequest pnpRequest, final return response; } catch (final IOException e) { - log.error("Failed to create temporary directory for ping and pull import", e); - throw new InvalidUserInputError("Failed to create temporary directory: " + e.getMessage(), e); + // This covers staging file system access anywhere in the operation, not just the creation of + // the temporary directory, so the message must not name a single step. + log.error("File system error during ping and pull import", e); + throw new InvalidUserInputError( + "File system error during ping and pull import: " + e.getMessage(), e); } catch (final Exception e) { log.error("Ping and pull import failed", e); final String errorMessage = extractRootCauseMessage(e); @@ -219,13 +223,15 @@ String resolveStagingBaseUri(@Nonnull final PnpConfiguration pnpConfig) { * @param pnpRequest the ping and pull import request * @param pnpConfig the PnP configuration * @param outputDir the directory where downloaded files will be written + * @param fs the file system that downloads should be written through * @return the configured bulk export client */ @Nonnull BulkExportClient buildBulkExportClient( @Nonnull final ImportPnpRequest pnpRequest, @Nonnull final PnpConfiguration pnpConfig, - @Nonnull final Path outputDir) { + @Nonnull final Path outputDir, + @Nonnull final FileSystem fs) { // Static mode is not currently supported. if ("static".equals(pnpRequest.exportType())) { @@ -260,13 +266,23 @@ BulkExportClient buildBulkExportClient( authConfig = authBuilder.build(); } - // Build the client. The Hadoop FileStore factory routes writes through the same scheme - // handlers as the rest of the server, allowing s3a://, hdfs:// and other warehouses. + // Build the client. Downloads go through the server's own file system, so that they use the + // same scheme handlers as the rest of the server, and so that the file system is still open + // for the staging directory to be listed once the download is complete. Lending the file + // system also avoids opening a second one for every import. + // Downloads are written to storage as they are received, so a download that is waiting on a + // slow write is not reading from its connection. Keep the number of concurrent downloads and + // the socket timeout in step with what the storage can sustain, or connections are closed + // part-way through a file. + final HttpClientConfig httpClientConfig = + HttpClientConfig.builder().socketTimeout(pnpConfig.getDownloadSocketTimeout()).build(); final var clientBuilder = BulkExportClient.systemBuilder() .withFhirEndpointUrl(pnpRequest.exportUrl()) .withOutputDir(outputDir.toString()) - .withFileStoreFactory(new HdfsFileStoreFactory(hadoopConfiguration)); + .withFileStoreFactory(HdfsFileStoreFactory.forFileSystem(fs)) + .withHttpClientConfig(httpClientConfig) + .withMaxConcurrentDownloads(pnpConfig.getMaxConcurrentDownloads()); if (authConfig != null) { clientBuilder.withAuthConfig(authConfig); diff --git a/server/src/main/java/au/csiro/pathling/operations/bulksubmit/BulkSubmitExecutor.java b/server/src/main/java/au/csiro/pathling/operations/bulksubmit/BulkSubmitExecutor.java index 9e16b3fc86..71ff3c0482 100644 --- a/server/src/main/java/au/csiro/pathling/operations/bulksubmit/BulkSubmitExecutor.java +++ b/server/src/main/java/au/csiro/pathling/operations/bulksubmit/BulkSubmitExecutor.java @@ -161,8 +161,11 @@ public BulkSubmitExecutor( * @param manifestJob The manifest job to process. * @param fileRequestHeaders Custom HTTP headers to include when downloading files. * @param fhirServerBase The FHIR server base URL for building result manifests. + * @return A future that completes when the asynchronous download work has reached a terminal + * state, allowing callers to await it. */ - public void downloadManifestJob( + @Nonnull + public CompletableFuture downloadManifestJob( @Nonnull final Submission submission, @Nonnull final ManifestJob manifestJob, @Nonnull final List fileRequestHeaders, @@ -178,6 +181,10 @@ public void downloadManifestJob( // Create and register the Job. final Optional ownerId = Optional.ofNullable(submission.ownerId()); final Job job = new Job<>(jobId, "bulk-submit-manifest", resultFuture, ownerId); + // This job's work is not run by the asynchronous request machinery, so no thread will ever + // signal its termination. Marking it terminated up front is what lets a deletion request do + // its own clean-up, rather than defer it to a signal that never arrives. + job.markTerminated(); jobRegistry.register(job); // Store the job ID in the manifest job. @@ -197,7 +204,7 @@ public void downloadManifestJob( } // Execute asynchronously to not block the request thread. - CompletableFuture.runAsync( + return CompletableFuture.runAsync( () -> downloadManifestJobInternal( submission, manifestJob, fileRequestHeaders, fhirServerBase, jobId, resultFuture)); diff --git a/server/src/main/java/au/csiro/pathling/operations/export/ExportDataSourceBuilder.java b/server/src/main/java/au/csiro/pathling/operations/export/ExportDataSourceBuilder.java new file mode 100644 index 0000000000..97a2b9e3dd --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/export/ExportDataSourceBuilder.java @@ -0,0 +1,109 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.export; + +import au.csiro.pathling.library.io.source.QueryableDataSource; +import au.csiro.pathling.operations.compartment.PatientCompartmentService; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.util.Set; +import org.hl7.fhir.r4.model.InstantType; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Component; + +/** + * Builds the filtered data source for an asynchronous export: applies the {@code _since} + * (updated-since) filter and the patient-compartment filter derived from the {@code patient} and + * {@code group} parameters. Shared by both {@code $viewdefinition-export} and {@code + * $sqlquery-export} so the two operations scope exported rows identically. + * + * @author John Grimes + */ +@Component +public class ExportDataSourceBuilder { + + @Nonnull private final PatientCompartmentService patientCompartmentService; + + /** + * Constructs a new ExportDataSourceBuilder. + * + * @param patientCompartmentService the patient compartment service used for row-level filtering + */ + @Autowired + public ExportDataSourceBuilder( + @Nonnull final PatientCompartmentService patientCompartmentService) { + this.patientCompartmentService = patientCompartmentService; + } + + /** + * Applies the export filters to the base data source. + * + * @param base the unfiltered data source + * @param since the {@code _since} filter, or null for no time filter + * @param patientIds the patient ids (resolved from {@code patient} and {@code group}); empty for + * no compartment filter + * @return the filtered data source + */ + @Nonnull + public QueryableDataSource build( + @Nonnull final QueryableDataSource base, + @Nullable final InstantType since, + @Nonnull final Set patientIds) { + QueryableDataSource dataSource = base; + + // Apply the _since filter. + if (since != null) { + dataSource = + dataSource.map( + rowDataset -> + rowDataset.filter( + "meta.lastUpdated IS NULL OR meta.lastUpdated >= '" + + since.getValueAsString() + + "'")); + } + + // Apply the patient compartment filter if patient ids were specified. + if (!patientIds.isEmpty()) { + dataSource = applyPatientCompartmentFilter(dataSource, patientIds); + } + + return dataSource; + } + + /** + * Applies the patient compartment filter to the data source. Uses the FHIRPath-aware compartment + * filter so that non-Patient resource types whose compartment membership is defined by a FHIRPath + * expression (for example {@code Observation.subject.where(resolve() is Patient)}) are filtered + * correctly, not just resources with a flat reference column. + */ + @Nonnull + private QueryableDataSource applyPatientCompartmentFilter( + @Nonnull final QueryableDataSource base, @Nonnull final Set patientIds) { + + // Filter out resource types that are not in the Patient compartment. + final QueryableDataSource filtered = + base.filterByResourceType(patientCompartmentService::isInPatientCompartment); + + // Apply row-level filtering based on patient compartment membership, evaluating any FHIRPath + // compartment paths against the unfiltered source. + return filtered.map( + (resourceType, rowDataset) -> + patientCompartmentService.filterByPatientCompartment( + resourceType, patientIds, rowDataset, base)); + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/export/ExportFileWriter.java b/server/src/main/java/au/csiro/pathling/operations/export/ExportFileWriter.java new file mode 100644 index 0000000000..94e37d115f --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/export/ExportFileWriter.java @@ -0,0 +1,200 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.export; + +import static au.csiro.pathling.library.io.FileSystemPersistence.safelyJoinPaths; + +import au.csiro.pathling.io.JobDirectoryFileSystem; +import au.csiro.pathling.library.io.FileSystemPersistence; +import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import jakarta.annotation.Nonnull; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import lombok.extern.slf4j.Slf4j; +import org.apache.hadoop.fs.Path; +import org.apache.spark.sql.AnalysisException; +import org.apache.spark.sql.Dataset; +import org.apache.spark.sql.Row; +import org.apache.spark.sql.SaveMode; +import org.apache.spark.sql.SparkSession; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Component; + +/** + * Writes asynchronous-export result datasets to files under the per-job directory in the warehouse, + * and serves the per-job directory and unique-naming helpers. Shared by both {@code + * $viewdefinition-export} and {@code $sqlquery-export} so that the two operations write their + * outputs identically (the same directory layout, partition renaming, and CSV unsupported-type + * handling). + * + * @author John Grimes + */ +@Slf4j +@Component +public class ExportFileWriter { + + @Nonnull private final SparkSession sparkSession; + + @Nonnull private final JobDirectoryFileSystem jobDirectoryFileSystem; + + /** + * Constructs a new ExportFileWriter. + * + * @param sparkSession the Spark session used to write the output files + * @param jobDirectoryFileSystem the shared helper that resolves, creates, and deletes per-job + * directories on the warehouse filesystem + */ + @Autowired + public ExportFileWriter( + @Nonnull final SparkSession sparkSession, + @Nonnull final JobDirectoryFileSystem jobDirectoryFileSystem) { + this.sparkSession = sparkSession; + this.jobDirectoryFileSystem = jobDirectoryFileSystem; + } + + /** + * Creates the per-job directory under the warehouse for storing output files. The directory is + * created on the warehouse filesystem, resolved from the warehouse URI, so it works for any + * warehouse scheme. + * + * @param jobId the job id + * @return the qualified job directory path + */ + @Nonnull + public Path createJobDirectory(@Nonnull final String jobId) { + try { + jobDirectoryFileSystem.ensureJobDirectory(jobId); + return jobDirectoryFileSystem.jobDirectory(jobId); + } catch (final IOException e) { + throw new InternalErrorException( + "Failed to create job directory for job %s.".formatted(jobId), e); + } + } + + /** + * Deletes the per-job directory and all its contents, used to clean up partial outputs when an + * export fails. Failures to delete are logged and swallowed, since cleanup is best-effort. + * + * @param jobId the job id whose directory should be removed + */ + public void deleteJobDirectory(@Nonnull final String jobId) { + try { + jobDirectoryFileSystem.deleteJobDirectory(jobId); + log.debug("Deleted partial output dir for job {}", jobId); + } catch (final IOException e) { + log.warn("Failed to delete partial output directory for job {}", jobId, e); + } + } + + /** + * Returns a name unique among the already-used names, appending a numeric suffix on collision. + * + * @param baseName the desired base name + * @param usedNames the names already used in this export (not modified) + * @return a unique name + */ + @Nonnull + public String uniqueName(@Nonnull final String baseName, @Nonnull final Set usedNames) { + if (!usedNames.contains(baseName)) { + return baseName; + } + int suffix = 1; + while (usedNames.contains(baseName + "_" + suffix)) { + suffix++; + } + return baseName + "_" + suffix; + } + + /** + * Writes the result as NDJSON files, returning the resulting file URLs. + * + * @param result the result dataset + * @param name the output name (used as the file/directory base name) + * @param jobDirPath the per-job directory + * @return the written file URLs, one per partition + */ + @Nonnull + public List writeNdjson( + @Nonnull final Dataset result, + @Nonnull final String name, + @Nonnull final Path jobDirPath) { + final String outputPath = safelyJoinPaths(jobDirPath.toString(), name + ".ndjson"); + result.write().mode(SaveMode.Overwrite).json(outputPath); + return new ArrayList<>( + FileSystemPersistence.renamePartitionedFiles(sparkSession, outputPath, outputPath, "json")); + } + + /** + * Writes the result as CSV files, returning the resulting file URLs. + * + * @param result the result dataset + * @param name the output name (used as the file/directory base name) + * @param includeHeader whether to include a CSV header row + * @param jobDirPath the per-job directory + * @return the written file URLs, one per partition + * @throws InvalidRequestException if the dataset contains data types unsupported by CSV + */ + @Nonnull + public List writeCsv( + @Nonnull final Dataset result, + @Nonnull final String name, + final boolean includeHeader, + @Nonnull final Path jobDirPath) { + final String outputPath = safelyJoinPaths(jobDirPath.toString(), name + ".csv"); + try { + result.write().mode(SaveMode.Overwrite).option("header", includeHeader).csv(outputPath); + } catch (final Exception e) { + // Spark throws AnalysisException when it encounters unsupported data types for a datasource. + // We convert this to an InvalidRequestException to return a 400 status code. + if (e instanceof final AnalysisException ae + && "UNSUPPORTED_DATA_TYPE_FOR_DATASOURCE".equals(ae.getErrorClass())) { + throw new InvalidRequestException( + "CSV export failed for output '%s': %s".formatted(name, e.getMessage())); + } + if (e instanceof final RuntimeException re) { + throw re; + } + throw new RuntimeException(e); + } + return new ArrayList<>( + FileSystemPersistence.renamePartitionedFiles(sparkSession, outputPath, outputPath, "csv")); + } + + /** + * Writes the result as Parquet files, returning the resulting file URLs. + * + * @param result the result dataset + * @param name the output name (used as the file/directory base name) + * @param jobDirPath the per-job directory + * @return the written file URLs, one per partition + */ + @Nonnull + public List writeParquet( + @Nonnull final Dataset result, + @Nonnull final String name, + @Nonnull final Path jobDirPath) { + final String outputPath = safelyJoinPaths(jobDirPath.toString(), name + ".parquet"); + result.write().mode(SaveMode.Overwrite).parquet(outputPath); + return new ArrayList<>( + FileSystemPersistence.renamePartitionedFiles( + sparkSession, outputPath, outputPath, "parquet")); + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/export/ExportManifest.java b/server/src/main/java/au/csiro/pathling/operations/export/ExportManifest.java new file mode 100644 index 0000000000..65af533e59 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/export/ExportManifest.java @@ -0,0 +1,176 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.export; + +import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.net.URISyntaxException; +import java.time.Duration; +import java.time.Instant; +import java.util.Date; +import java.util.List; +import org.apache.http.client.utils.URIBuilder; +import org.hl7.fhir.r4.model.CodeType; +import org.hl7.fhir.r4.model.InstantType; +import org.hl7.fhir.r4.model.IntegerType; +import org.hl7.fhir.r4.model.Parameters; +import org.hl7.fhir.r4.model.Parameters.ParametersParameterComponent; +import org.hl7.fhir.r4.model.StringType; +import org.hl7.fhir.r4.model.UriType; + +/** + * Builds the SQL on FHIR asynchronous-export completion manifest as a FHIR {@code Parameters} + * resource. The manifest follows the shape shared by {@code $viewdefinition-export} and {@code + * $sqlquery-export}: a required {@code exportId} and {@code status}, the echoed {@code + * clientTrackingId} and {@code _format}, the export timing fields, and one {@code output} per + * exported unit with a {@code name} and one or more {@code location} download URLs. + * + *

The {@code cancelUrl} and {@code estimatedTimeRemaining} parameters are deliberately omitted, + * consistent with the agreed scope of both export operations. + * + * @author John Grimes + */ +public class ExportManifest { + + @Nonnull private final String serverBaseUrl; + + @Nonnull private final String exportId; + + @Nullable private final String clientTrackingId; + + @Nonnull private final String format; + + @Nonnull private final Instant exportStartTime; + + @Nonnull private final Instant exportEndTime; + + @Nonnull private final List outputs; + + /** + * Creates a new ExportManifest. + * + * @param serverBaseUrl the FHIR server base URL (used for constructing the result download URLs) + * @param exportId the server-assigned export job identifier + * @param clientTrackingId the client-supplied tracking identifier, or null if none was supplied + * @param format the effective output format code (e.g. {@code ndjson}) + * @param exportStartTime the export job creation (kick-off) time + * @param exportEndTime the time the manifest is built on completion + * @param outputs the export outputs, one per exported unit, in order + */ + @SuppressWarnings("java:S107") + public ExportManifest( + @Nonnull final String serverBaseUrl, + @Nonnull final String exportId, + @Nullable final String clientTrackingId, + @Nonnull final String format, + @Nonnull final Instant exportStartTime, + @Nonnull final Instant exportEndTime, + @Nonnull final List outputs) { + this.serverBaseUrl = serverBaseUrl; + this.exportId = exportId; + this.clientTrackingId = clientTrackingId; + this.format = format; + this.exportStartTime = exportStartTime; + this.exportEndTime = exportEndTime; + this.outputs = outputs; + } + + /** + * Builds the completion manifest as a FHIR {@code Parameters} resource. + * + * @return the manifest Parameters + */ + @Nonnull + public Parameters toParameters() { + final Parameters parameters = new Parameters(); + + // Ensure the base URL ends with a slash for proper URL construction. + final String normalizedBaseUrl = + serverBaseUrl.endsWith("/") ? serverBaseUrl : serverBaseUrl + "/"; + + parameters.addParameter().setName("exportId").setValue(new StringType(exportId)); + parameters.addParameter().setName("status").setValue(new CodeType("completed")); + + // Echo the client tracking id only when one was supplied at kick-off. + if (clientTrackingId != null && !clientTrackingId.isBlank()) { + parameters + .addParameter() + .setName("clientTrackingId") + .setValue(new StringType(clientTrackingId)); + } + + parameters.addParameter().setName("_format").setValue(new CodeType(format)); + + parameters + .addParameter() + .setName("exportStartTime") + .setValue(new InstantType(Date.from(exportStartTime))); + parameters + .addParameter() + .setName("exportEndTime") + .setValue(new InstantType(Date.from(exportEndTime))); + + // Whole seconds between start and end, never negative. + final long durationSeconds = + Math.max(0L, Duration.between(exportStartTime, exportEndTime).toSeconds()); + parameters + .addParameter() + .setName("exportDuration") + .setValue(new IntegerType((int) durationSeconds)); + + // One output per exported unit, with a name and one or more location parts. + for (final ExportManifestOutput output : outputs) { + final ParametersParameterComponent outputParam = parameters.addParameter().setName("output"); + outputParam.addPart().setName("name").setValue(new StringType(output.name())); + for (final String fileUrl : output.fileUrls()) { + outputParam + .addPart() + .setName("location") + .setValue(new UriType(buildResultUrl(normalizedBaseUrl, fileUrl))); + } + } + + return parameters; + } + + /** + * Converts a local file URL to a remote {@code $result} download URL. + * + * @param baseUrl the normalised base server URL (ending in a slash) + * @param localUrl the local file URL containing the job id and filename + * @return the remote result URL + */ + @Nonnull + private static String buildResultUrl( + @Nonnull final String baseUrl, @Nonnull final String localUrl) { + try { + final String[] parts = localUrl.split("/jobs/")[1].split("/"); + final String jobUuid = parts[0]; + final String file = parts[1]; + + return new URIBuilder(baseUrl + "$result") + .addParameter("job", jobUuid) + .addParameter("file", file) + .build() + .toString(); + } catch (final URISyntaxException e) { + throw new InternalErrorException(e); + } + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/export/ExportManifestOutput.java b/server/src/main/java/au/csiro/pathling/operations/export/ExportManifestOutput.java new file mode 100644 index 0000000000..a3d7b85eb8 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/export/ExportManifestOutput.java @@ -0,0 +1,33 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.export; + +import jakarta.annotation.Nonnull; +import java.util.List; + +/** + * One finished export unit within a completion manifest: a friendly name and one or more + * downloadable file locations (local file URLs that the manifest builder maps to {@code $result} + * download URLs). Shared by both the {@code $viewdefinition-export} and {@code $sqlquery-export} + * operations, which produce the identical manifest shape (one output per exported unit). + * + * @param name the output name (one per exported unit) + * @param fileUrls the local file URLs for this output, in order; repeats once per partitioned file + * @author John Grimes + */ +public record ExportManifestOutput(@Nonnull String name, @Nonnull List fileUrls) {} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/CanonicalReference.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/CanonicalReference.java new file mode 100644 index 0000000000..61a5cca697 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/CanonicalReference.java @@ -0,0 +1,180 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.util.Comparator; +import java.util.List; +import java.util.Objects; +import java.util.function.Function; +import java.util.function.Predicate; + +/** + * A canonical reference parsed into its {@code url} and optional {@code version}, with the + * candidate-selection rule shared by the {@code Library} and {@code ViewDefinition} resolution + * paths. A canonical reference takes the form {@code [url]} or {@code [url]|[version]}; matching is + * always against the candidate resource's {@code url} element, never its logical id. + * + *

This is the single place the SQL on FHIR resolution chain parses a canonical and selects among + * candidates sharing a url: + * + *

    + *
  • with an explicit version, the caller has already filtered to that exact version, so any + * remaining candidate suffices; + *
  • without a version, the latest active match wins - preferring {@code status = active}, then + * the lexicographically greatest version string (FHIR does not constrain the shape of a + * version, so this is a reasonable proxy for "latest"). + *
+ * + * @author John Grimes + */ +public final class CanonicalReference { + + @Nonnull private final String url; + + @Nullable private final String version; + + private CanonicalReference(@Nonnull final String url, @Nullable final String version) { + this.url = url; + this.version = version; + } + + /** + * Parses a canonical reference of the form {@code [url]} or {@code [url]|[version]}. An empty + * version suffix (a trailing {@code |}) is treated as no version. + * + * @param canonical the canonical reference to parse + * @return the parsed reference + * @throws InvalidRequestException if the url segment is blank + */ + @Nonnull + public static CanonicalReference parse(@Nonnull final String canonical) { + final int pipe = canonical.indexOf('|'); + final String url = pipe >= 0 ? canonical.substring(0, pipe) : canonical; + final String rawVersion = pipe >= 0 ? canonical.substring(pipe + 1) : null; + if (url.isBlank()) { + throw new InvalidRequestException( + "Canonical reference '" + canonical + "' is missing the url segment"); + } + final String version = rawVersion != null && !rawVersion.isBlank() ? rawVersion : null; + return new CanonicalReference(url, version); + } + + /** + * Indicates whether a value is an absolute canonical URL acceptable as a dependency reference: it + * must use the {@code http://}, {@code https://}, or {@code urn:} scheme, may carry at most one + * {@code |version} suffix, and must not carry a fragment ({@code #...}). + * + * @param value the value to test + * @return {@code true} if the value is an absolute canonical URL + */ + public static boolean isCanonical(@Nullable final String value) { + if (value == null || value.isBlank()) { + return false; + } + if (value.indexOf('#') >= 0) { + // Fragments are not supported. + return false; + } + final int pipe = value.indexOf('|'); + if (pipe >= 0 && value.indexOf('|', pipe + 1) >= 0) { + // At most one version suffix is permitted. + return false; + } + final String url = pipe >= 0 ? value.substring(0, pipe) : value; + return url.startsWith("http://") || url.startsWith("https://") || url.startsWith("urn:"); + } + + /** + * Computes the canonical key identifying a resolved resource: its {@code url} plus its {@code + * version} when it has one ({@code url|version}), else the bare {@code url}. A bare-url reference + * and an explicit {@code url|version} reference that resolve to the same stored resource share + * this key, so the resource de-duplicates and materialises once. + * + * @param url the resolved resource's url + * @param version the resolved resource's version, if any + * @return the canonical key + */ + @Nonnull + public static String key(@Nonnull final String url, @Nullable final String version) { + return version != null && !version.isBlank() ? url + "|" + version : url; + } + + /** + * Selects the most appropriate candidate among those already matched on this reference's url. + * When this reference carries a version, the caller has already filtered to that exact version, + * so the first candidate is returned. Otherwise the latest active candidate wins: preferring + * {@code active} status, then the greatest version string. + * + * @param candidates the candidates already filtered to share this reference's url (and version, + * when one was supplied); must be non-empty + * @param isActive predicate identifying a candidate with {@code active} status + * @param versionOf extracts a candidate's version string (may return {@code null}) + * @param the candidate resource type + * @return the selected candidate + * @throws IllegalStateException if no candidates are supplied + */ + @Nonnull + public T select( + @Nonnull final List candidates, + @Nonnull final Predicate isActive, + @Nonnull final Function versionOf) { + if (candidates.isEmpty()) { + throw new IllegalStateException("Cannot select from an empty candidate list"); + } + if (candidates.size() == 1 || version != null) { + return candidates.get(0); + } + return candidates.stream() + .max( + Comparator.comparing(isActive::test) + .thenComparing(candidate -> Objects.toString(versionOf.apply(candidate), ""))) + .orElseThrow(() -> new IllegalStateException("Cannot select from an empty candidate list")); + } + + /** + * Returns the url segment of this canonical reference. + * + * @return the non-blank url + */ + @Nonnull + public String getUrl() { + return url; + } + + /** + * Returns the version segment of this canonical reference, if one was supplied. + * + * @return the version, or {@code null} when none was supplied + */ + @Nullable + public String getVersion() { + return version; + } + + /** + * Indicates whether this canonical reference carries an explicit version. + * + * @return {@code true} if a version was supplied + */ + public boolean hasVersion() { + return version != null; + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/LibraryReferenceResolver.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/LibraryReferenceResolver.java index 335d0c1312..72bf5d880b 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/LibraryReferenceResolver.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/LibraryReferenceResolver.java @@ -17,17 +17,19 @@ package au.csiro.pathling.operations.sqlquery; +import au.csiro.pathling.config.ServerConfiguration; import au.csiro.pathling.encoders.FhirEncoders; import au.csiro.pathling.errors.ResourceNotFoundError; import au.csiro.pathling.io.source.DataSource; import au.csiro.pathling.read.ReadExecutor; +import au.csiro.pathling.security.PathlingAuthority; +import au.csiro.pathling.security.ResourceAccess.AccessType; +import au.csiro.pathling.security.SecurityAspect; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; import jakarta.annotation.Nonnull; -import jakarta.annotation.Nullable; -import java.util.Comparator; import java.util.List; -import java.util.Objects; +import java.util.Optional; import org.apache.spark.sql.Dataset; import org.apache.spark.sql.Row; import org.apache.spark.sql.catalyst.encoders.ExpressionEncoder; @@ -48,6 +50,8 @@ *
  • Canonical references such as {@code https://example.org/Library/foo} or {@code * https://example.org/Library/foo|1.2}, matched against {@code Library.url}. * + * + * @author John Grimes */ @Component public class LibraryReferenceResolver { @@ -60,25 +64,32 @@ public class LibraryReferenceResolver { @Nonnull private final FhirEncoders fhirEncoders; + @Nonnull private final ServerConfiguration serverConfiguration; + /** * Constructs a new LibraryReferenceResolver. * * @param readExecutor used for relative-reference reads * @param dataSource the data source used for canonical-reference search * @param fhirEncoders FHIR encoders used to decode the search result rows + * @param serverConfiguration the server configuration (used for the auth toggle) */ @Autowired public LibraryReferenceResolver( @Nonnull final ReadExecutor readExecutor, @Nonnull final DataSource dataSource, - @Nonnull final FhirEncoders fhirEncoders) { + @Nonnull final FhirEncoders fhirEncoders, + @Nonnull final ServerConfiguration serverConfiguration) { this.readExecutor = readExecutor; this.dataSource = dataSource; this.fhirEncoders = fhirEncoders; + this.serverConfiguration = serverConfiguration; } /** - * Resolves the reference to a stored Library resource. + * Resolves the reference to a stored Library resource. As the Library is read from server + * storage, the metadata READ check on {@code Library} is enforced when authorisation is enabled, + * regardless of the operation that triggered the read. * * @param reference the reference to resolve; must carry a non-blank {@code reference} value * @return the resolved Library resource @@ -93,6 +104,10 @@ public IBaseResource resolve(@Nonnull final Reference reference) { "queryReference must carry a non-blank Reference.reference value"); } + if (serverConfiguration.getAuth().isEnabled()) { + SecurityAspect.checkHasAuthority(PathlingAuthority.resourceAccess(AccessType.READ, LIBRARY)); + } + if (isCanonical(ref)) { return resolveCanonical(ref); } @@ -142,24 +157,20 @@ private IBaseResource resolveRelative(@Nonnull final String ref) { } /** - * Resolves a canonical reference of the form {@code [url]} or {@code [url]|[version]}. When - * multiple matches exist, prefers an exact {@code url|version} match, then the latest active - * version (by {@code Library.version} string ordering, since FHIR doesn't constrain its shape). + * Resolves a canonical reference of the form {@code [url]} or {@code [url]|[version]}, matching + * against {@code Library.url}. The url/version split and candidate selection (exact {@code + * url|version}, else latest active by status then version string) are delegated to the shared + * {@link CanonicalReference} helper, so a {@code Library} and a {@code ViewDefinition} are + * selected by identical rules. */ @Nonnull private IBaseResource resolveCanonical(@Nonnull final String canonical) { - final int pipe = canonical.indexOf('|'); - final String url = pipe >= 0 ? canonical.substring(0, pipe) : canonical; - final String version = pipe >= 0 ? canonical.substring(pipe + 1) : null; - - if (url.isBlank()) { - throw new InvalidRequestException("queryReference canonical is missing the url segment"); - } + final CanonicalReference reference = CanonicalReference.parse(canonical); final Dataset libraries = dataSource.read(LIBRARY); - Dataset filtered = libraries.filter(libraries.col("url").equalTo(url)); - if (version != null && !version.isBlank()) { - filtered = filtered.filter(functions.col("version").equalTo(version)); + Dataset filtered = libraries.filter(libraries.col("url").equalTo(reference.getUrl())); + if (reference.hasVersion()) { + filtered = filtered.filter(functions.col("version").equalTo(reference.getVersion())); } final ExpressionEncoder encoder = fhirEncoders.of(LIBRARY); @@ -168,30 +179,60 @@ private IBaseResource resolveCanonical(@Nonnull final String canonical) { throw new ResourceNotFoundException( "Library with canonical reference '" + canonical + "' not found"); } - return pickBestCandidate(candidates, version); + return reference.select( + candidates, + candidate -> ((Library) candidate).getStatus() == PublicationStatus.ACTIVE, + candidate -> ((Library) candidate).getVersion()); } /** - * Selects the most appropriate Library when multiple match the canonical url. With a version - * suffix all matches already share that version, so any candidate suffices. Without a version - * suffix, prefer active over draft/retired, then take the lexicographically greatest version - * string (a reasonable proxy for "latest" given FHIR's freeform versioning). + * Attempts to resolve a canonical reference to a stored {@code SQLView} {@code Library} by + * matching {@code Library.url}, returning empty when no Library matches. Used by {@link + * SqlDependencyResolver} for the SQLView arm of canonical dependency resolution, where a + * non-match is not an error in itself (the reference may instead name a {@code ViewDefinition}). + * + *

    The candidate-selection rules match {@link #resolveCanonical}: an exact {@code url|version} + * match, else the latest active version. The {@code Library} metadata READ check is enforced + * (when authorisation is enabled) only once a Library is actually matched, so referencing a URL + * that names a {@code ViewDefinition} the caller can read is not blocked by missing {@code + * Library} authority. + * + * @param canonical the canonical reference to resolve + * @return the resolved Library, or empty if no Library matches the canonical url */ @Nonnull - private IBaseResource pickBestCandidate( - @Nonnull final List candidates, @Nullable final String version) { - if (candidates.size() == 1 || version != null) { - return candidates.get(0); + public Optional tryResolveSqlViewLibrary(@Nonnull final String canonical) { + final CanonicalReference reference = CanonicalReference.parse(canonical); + + final Dataset libraries; + try { + libraries = dataSource.read(LIBRARY); + } catch (final IllegalArgumentException e) { + // The server holds no Library data at all, so the reference simply does not match a SQLView. + if (e.getMessage() != null && e.getMessage().contains("No data found for resource type")) { + return Optional.empty(); + } + throw e; + } + Dataset filtered = libraries.filter(libraries.col("url").equalTo(reference.getUrl())); + if (reference.hasVersion()) { + filtered = filtered.filter(functions.col("version").equalTo(reference.getVersion())); + } + + final ExpressionEncoder encoder = fhirEncoders.of(LIBRARY); + final List candidates = filtered.as(encoder).collectAsList(); + if (candidates.isEmpty()) { + return Optional.empty(); + } + + if (serverConfiguration.getAuth().isEnabled()) { + SecurityAspect.checkHasAuthority(PathlingAuthority.resourceAccess(AccessType.READ, LIBRARY)); } - return candidates.stream() - .map(Library.class::cast) - .max( - Comparator.comparing((Library lib) -> lib.getStatus() == PublicationStatus.ACTIVE) - .thenComparing(lib -> Objects.toString(lib.getVersion(), ""))) - .map(IBaseResource.class::cast) - .orElseThrow( - () -> - new ResourceNotFoundException( - "Library with canonical reference could not be selected")); + final IBaseResource selected = + reference.select( + candidates, + candidate -> ((Library) candidate).getStatus() == PublicationStatus.ACTIVE, + candidate -> ((Library) candidate).getVersion()); + return Optional.of((Library) selected); } } diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/ParsedSqlQuery.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ParsedSqlQuery.java index 9af7985135..6f8acdb99b 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/ParsedSqlQuery.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ParsedSqlQuery.java @@ -22,8 +22,10 @@ import lombok.Value; /** - * Represents a parsed SQLQuery Library resource containing the SQL text, ViewDefinition - * dependencies, and declared parameters. + * Represents a parsed SQL on FHIR Library resource ({@code SQLQuery} or {@code SQLView}) containing + * the SQL text, dependency references, declared parameters, and the resolved library type code. + * + * @author John Grimes */ @Value public class ParsedSqlQuery { @@ -31,9 +33,24 @@ public class ParsedSqlQuery { /** The decoded SQL query text. */ @Nonnull String sql; - /** The ViewDefinition dependencies referenced in the SQL query. */ + /** The dependency references (to ViewDefinitions and/or SQLViews) referenced in the SQL. */ @Nonnull List viewReferences; - /** The declared parameters that can be bound at execution time. */ + /** The declared parameters that can be bound at execution time. Always empty for a SQLView. */ @Nonnull List declaredParameters; + + /** + * The SQL on FHIR library type code: {@link SqlLibraryParser#SQL_QUERY_TYPE_CODE} or {@link + * SqlLibraryParser#SQL_VIEW_TYPE_CODE}. + */ + @Nonnull String libraryTypeCode; + + /** + * Indicates whether this parsed query came from a {@code SQLView} Library. + * + * @return {@code true} if the library type is {@code sql-view} + */ + public boolean isView() { + return SqlLibraryParser.isView(libraryTypeCode); + } } diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/PreparedSqlQuery.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/PreparedSqlQuery.java new file mode 100644 index 0000000000..12b2963613 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/PreparedSqlQuery.java @@ -0,0 +1,42 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import jakarta.annotation.Nonnull; +import lombok.Value; + +/** + * A SQL query that has been parsed and had its dependency graph resolved, ready for static + * validation and execution by {@link SqlQueryPipeline}. Produced by {@link + * SqlQueryPipeline#prepare}; shared by the synchronous {@code $sqlquery-run} and the asynchronous + * {@code $sqlquery-export} operations. + * + * @author John Grimes + */ +@Value +public class PreparedSqlQuery { + + /** The validated, normalised request: parsed query, output format, header flag, bindings. */ + @Nonnull SqlQueryRequest request; + + /** + * The resolved dependency graph the top-level SQL references: the transitive set of + * ViewDefinition and SQLView nodes, topologically ordered for materialisation. + */ + @Nonnull ResolvedDependencyGraph dependencyGraph; +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/QueryInput.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/QueryInput.java new file mode 100644 index 0000000000..c162ddf727 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/QueryInput.java @@ -0,0 +1,54 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; + +/** + * One {@code query} repetition of a {@code $sqlquery-export} request, prepared (parsed, parameter + * bound, and view resolved) at kick-off and carried to background execution. Each query input + * produces exactly one export output. + * + * @param name the optional {@code query.name}, the highest-precedence output name + * @param libraryName the SQLQuery Library's {@code name} element, used as the output-name fallback + * @param preparedQuery the prepared query (parsed SQL, bound parameters, resolved views) + * @author John Grimes + */ +public record QueryInput( + @Nullable String name, @Nullable String libraryName, @Nonnull PreparedSqlQuery preparedQuery) { + + /** + * Derives the output name for this query: the {@code query.name} when supplied, otherwise the + * SQLQuery Library's {@code name} element, otherwise a generated name based on the index. Names + * are made unique across the export by the executor. + * + * @param index the index of this query in the request (used for the generated fallback) + * @return the effective output name before uniqueness is applied + */ + @Nonnull + public String getEffectiveName(final int index) { + if (name != null && !name.isBlank()) { + return name; + } + if (libraryName != null && !libraryName.isBlank()) { + return libraryName; + } + return "query_" + index; + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/ResolvedDependency.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ResolvedDependency.java new file mode 100644 index 0000000000..5dcb94f9fa --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ResolvedDependency.java @@ -0,0 +1,40 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import jakarta.annotation.Nonnull; + +/** + * A resolved node in a SQL on FHIR dependency graph: either a {@link ResolvedViewDefinition} leaf + * or a {@link ResolvedSqlView}. Each node is identified by a stable canonical key that is the basis + * of its request-scoped temp-view name and of diamond deduplication. + * + * @author John Grimes + */ +public interface ResolvedDependency { + + /** + * Returns the stable canonical identity of the resolved resource. Two references to the same + * resource share a key, so a node is materialised only once per request, and the key cannot + * collide with a different resource even when both are reached under the same table label. + * + * @return the canonical key + */ + @Nonnull + String getCanonicalKey(); +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/ResolvedDependencyGraph.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ResolvedDependencyGraph.java new file mode 100644 index 0000000000..b3de661d8a --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ResolvedDependencyGraph.java @@ -0,0 +1,47 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import jakarta.annotation.Nonnull; +import java.util.List; +import java.util.Map; +import lombok.Value; + +/** + * The fully resolved dependency graph for a single query: the transitive set of {@link + * ResolvedDependency} nodes reachable from the top-level query, together with the mapping from the + * top-level query's own table labels to the nodes they reference. Produced during request + * preparation (no Spark) and materialised, bottom-up, at execution. + * + * @author John Grimes + */ +@Value +public class ResolvedDependencyGraph { + + /** + * The nodes in topological order: every node appears after all of its dependencies, so + * materialising the list in order guarantees each node's children already exist as temp views. + */ + @Nonnull List orderedNodes; + + /** The top-level query's local table label to the canonical key of the node it references. */ + @Nonnull Map topLevelKeysByLabel; + + /** Lookup of every node by its canonical key, for deduplication and materialisation. */ + @Nonnull Map nodesByKey; +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/ResolvedSqlView.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ResolvedSqlView.java new file mode 100644 index 0000000000..f9ac5bc5ce --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ResolvedSqlView.java @@ -0,0 +1,42 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import jakarta.annotation.Nonnull; +import java.util.Map; +import lombok.Value; + +/** + * A resolved {@code SQLView} node. Carries the view's SQL and the mapping from each table label the + * SQL uses to the canonical key of the child node that label resolves to, so the SQL can be + * rewritten against the children's request-scoped temp views at materialisation time. + * + * @author John Grimes + */ +@Value +public class ResolvedSqlView implements ResolvedDependency { + + /** The stable canonical identity of the SQLView Library. */ + @Nonnull String canonicalKey; + + /** The view's SQL text. */ + @Nonnull String sql; + + /** This view's local table label to the canonical key of the resolved child it references. */ + @Nonnull Map childKeysByLabel; +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/ResolvedViewDefinition.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ResolvedViewDefinition.java new file mode 100644 index 0000000000..bb019e2542 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ResolvedViewDefinition.java @@ -0,0 +1,41 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import au.csiro.pathling.views.FhirView; +import jakarta.annotation.Nonnull; +import lombok.Value; + +/** + * A resolved leaf node wrapping a parsed {@link FhirView}. A {@code ViewDefinition} projects a + * single FHIR resource type and never declares further dependencies, so it is always a leaf of the + * dependency graph. + * + * @author John Grimes + */ +@Value +public class ResolvedViewDefinition implements ResolvedDependency { + + /** The stable canonical identity of the ViewDefinition. */ + @Nonnull String canonicalKey; + + /** + * The parsed view, ready for execution. Its resource drives the projected-resource READ check. + */ + @Nonnull FhirView view; +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlDependencyResolver.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlDependencyResolver.java new file mode 100644 index 0000000000..c869d31956 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlDependencyResolver.java @@ -0,0 +1,266 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import au.csiro.pathling.config.ServerConfiguration; +import au.csiro.pathling.views.FhirView; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; +import jakarta.annotation.Nonnull; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import org.hl7.fhir.r4.model.Library; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Component; + +/** + * Resolves the transitive dependency graph of a top-level query (a {@code SQLQuery} or a {@code + * SQLView}) into a {@link ResolvedDependencyGraph}, without touching Spark. Each {@code + * relatedArtifact} dependency is resolved by canonical URL, authorised, and parsed; {@code SQLView} + * dependencies are recursed into so the full graph of virtual tables is resolved. + * + *

    Reference resolution follows the SQL on FHIR canonical-reference contract: a {@code + * relatedArtifact.resource} is an absolute canonical URL (optionally {@code |version}), matched + * against the candidate resource's {@code url} - never its logical id. For a reference the + * resolver: + * + *

      + *
    1. prefers a request-supplied view whose URL matches; + *
    2. otherwise searches stored {@code ViewDefinition}s by url, then {@code SQLView Library}s by + * url; + *
    3. rejects a URL that matches both a ViewDefinition and a SQLView as ambiguous, and a URL that + * matches neither as not found - each naming the label and the reference. + *
    + * + *

    The resolution memoises by the resolved canonical key (the matched resource's url plus its + * version, else the bare url), so a node referenced from more than one place (a diamond) - + * including a bare-url reference and a {@code url|version} reference to the same stored resource - + * is resolved once and shared. A reference encountered while it is already on the resolution stack + * is a cycle and is rejected, as is a graph that nests deeper than the configured {@code + * maxDependencyDepth}. All such failures are reported before any Spark execution. + * + * @author John Grimes + */ +@Component +public class SqlDependencyResolver { + + @Nonnull private final ViewResolver viewResolver; + + @Nonnull private final LibraryReferenceResolver libraryReferenceResolver; + + @Nonnull private final SqlLibraryParser libraryParser; + + @Nonnull private final ServerConfiguration serverConfiguration; + + /** + * Constructs a new SqlDependencyResolver. + * + * @param viewResolver resolves ViewDefinition leaves by url, preferring request-supplied views + * @param libraryReferenceResolver resolves a SQLView Library by canonical url from storage + * @param libraryParser the shared parser for SQLView Libraries + * @param serverConfiguration the server configuration (auth toggle and the dependency depth cap) + */ + @Autowired + public SqlDependencyResolver( + @Nonnull final ViewResolver viewResolver, + @Nonnull final LibraryReferenceResolver libraryReferenceResolver, + @Nonnull final SqlLibraryParser libraryParser, + @Nonnull final ServerConfiguration serverConfiguration) { + this.viewResolver = viewResolver; + this.libraryReferenceResolver = libraryReferenceResolver; + this.libraryParser = libraryParser; + this.serverConfiguration = serverConfiguration; + } + + /** + * Resolves the dependency graph for a parsed top-level query. + * + * @param topLevel the parsed top-level query (SQLQuery or SQLView) + * @param suppliedViews request-supplied views keyed by the canonical URL they satisfy, used for + * the top-level query's direct references; nested SQLView dependencies resolve from storage + * only + * @return the resolved dependency graph, topologically ordered + * @throws InvalidRequestException if a reference is ambiguous, a cycle or depth-limit breach is + * detected, or a dependency is a malformed or wrong-typed resource + * @throws ResourceNotFoundException if a reference matches no stored ViewDefinition or SQLView + */ + @Nonnull + public ResolvedDependencyGraph resolve( + @Nonnull final ParsedSqlQuery topLevel, @Nonnull final Map suppliedViews) { + final int maxDepth = serverConfiguration.getSqlQuery().getMaxDependencyDepth(); + final Map nodesByKey = new LinkedHashMap<>(); + final Set resolutionStack = new LinkedHashSet<>(); + final Map topLevelKeysByLabel = + resolveReferences( + topLevel.getViewReferences(), suppliedViews, 1, maxDepth, resolutionStack, nodesByKey); + return new ResolvedDependencyGraph( + new ArrayList<>(nodesByKey.values()), topLevelKeysByLabel, nodesByKey); + } + + /** + * Resolves a list of references in order, returning their labels mapped to the canonical keys of + * the nodes they resolve to. + */ + @Nonnull + private Map resolveReferences( + @Nonnull final List references, + @Nonnull final Map suppliedViews, + final int depth, + final int maxDepth, + @Nonnull final Set resolutionStack, + @Nonnull final Map nodesByKey) { + final Map keysByLabel = new LinkedHashMap<>(); + for (final ViewArtifactReference reference : references) { + keysByLabel.put( + reference.getLabel(), + resolveReference(reference, suppliedViews, depth, maxDepth, resolutionStack, nodesByKey)); + } + return keysByLabel; + } + + /** + * Resolves a single reference into the canonical key of its node, registering it if new. A + * request-supplied view wins; otherwise the canonical url is matched against stored + * ViewDefinitions then SQLView Libraries, rejecting an ambiguous match (both types) and a + * not-found match (neither type). + */ + @Nonnull + private String resolveReference( + @Nonnull final ViewArtifactReference reference, + @Nonnull final Map suppliedViews, + final int depth, + final int maxDepth, + @Nonnull final Set resolutionStack, + @Nonnull final Map nodesByKey) { + if (depth > maxDepth) { + throw new InvalidRequestException( + "Dependency graph nests deeper than the configured maximum of " + + maxDepth + + " (at label '" + + reference.getLabel() + + "', reference '" + + reference.getCanonicalUrl() + + "')"); + } + + // A request-supplied view, matched by url, is preferred over storage. + final Optional suppliedView = + viewResolver.resolveSuppliedView(reference, suppliedViews); + if (suppliedView.isPresent()) { + return registerLeaf(suppliedView.get(), nodesByKey); + } + + // Search stored ViewDefinitions, then stored SQLView Libraries, both by url. + final Optional storedViewDefinition = + viewResolver.resolveStoredViewDefinition(reference); + final Optional sqlViewLibrary = + libraryReferenceResolver.tryResolveSqlViewLibrary(reference.getCanonicalUrl()); + + if (storedViewDefinition.isPresent() && sqlViewLibrary.isPresent()) { + throw new InvalidRequestException( + "The dependency for label '" + + reference.getLabel() + + "' (reference '" + + reference.getCanonicalUrl() + + "') is ambiguous: the canonical URL matches both a ViewDefinition and a SQLView"); + } + if (storedViewDefinition.isPresent()) { + return registerLeaf(storedViewDefinition.get(), nodesByKey); + } + if (sqlViewLibrary.isPresent()) { + return resolveSqlView( + sqlViewLibrary.get(), reference, depth, maxDepth, resolutionStack, nodesByKey); + } + throw new ResourceNotFoundException( + "Failed to resolve the dependency for label '" + + reference.getLabel() + + "' with reference '" + + reference.getCanonicalUrl() + + "': no ViewDefinition or SQLView matches that canonical URL"); + } + + /** Registers a resolved ViewDefinition leaf (deduplicating diamonds) and returns its key. */ + @Nonnull + private String registerLeaf( + @Nonnull final ResolvedViewDefinition leaf, + @Nonnull final Map nodesByKey) { + nodesByKey.putIfAbsent(leaf.getCanonicalKey(), leaf); + return leaf.getCanonicalKey(); + } + + /** + * Resolves a matched {@code SQLView} {@code Library}, recursing into its own dependencies. Keys + * the node by the resolved canonical (the library's url plus its version, else the bare url), so + * two references to the same stored Library - including a bare-url and a {@code url|version} + * reference - deduplicate. Detects diamonds (already resolved), cycles (currently on the + * resolution stack), and rejects a {@code sql-query} Library referenced as a dependency. + */ + @Nonnull + private String resolveSqlView( + @Nonnull final Library library, + @Nonnull final ViewArtifactReference reference, + final int depth, + final int maxDepth, + @Nonnull final Set resolutionStack, + @Nonnull final Map nodesByKey) { + final String canonicalKey = CanonicalReference.key(library.getUrl(), library.getVersion()); + + // A node already fully resolved is shared (diamond dedup). + if (nodesByKey.containsKey(canonicalKey)) { + return canonicalKey; + } + // A node still being resolved is a cycle. + if (resolutionStack.contains(canonicalKey)) { + throw new InvalidRequestException( + "Cyclic dependency detected: " + + String.join(" -> ", resolutionStack) + + " -> " + + canonicalKey); + } + + final ParsedSqlQuery parsed = libraryParser.parse(library); + if (!parsed.isView()) { + throw new InvalidRequestException( + "The dependency for label '" + + reference.getLabel() + + "' (reference '" + + reference.getCanonicalUrl() + + "') is a " + + parsed.getLibraryTypeCode() + + " Library, but only a SQLView may be referenced as a dependency"); + } + + resolutionStack.add(canonicalKey); + // Nested dependencies resolve from storage only; request-supplied views satisfy the top-level + // query's references, not the internals of a stored SQLView. + final Map childKeysByLabel = + resolveReferences( + parsed.getViewReferences(), Map.of(), depth + 1, maxDepth, resolutionStack, nodesByKey); + resolutionStack.remove(canonicalKey); + + final ResolvedSqlView node = + new ResolvedSqlView(canonicalKey, parsed.getSql(), childKeysByLabel); + nodesByKey.put(canonicalKey, node); + return canonicalKey; + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryLibraryParser.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlLibraryParser.java similarity index 51% rename from server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryLibraryParser.java rename to server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlLibraryParser.java index 28bdea09e4..fa6c1a67a7 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryLibraryParser.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlLibraryParser.java @@ -19,6 +19,7 @@ import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; @@ -34,99 +35,123 @@ import org.springframework.stereotype.Component; /** - * Parses a FHIR R4 Library resource conforming to the SQLQuery profile. Extracts the SQL text, - * ViewDefinition dependencies, and parameter declarations, and enforces the profile invariants. + * Parses a FHIR R4 Library resource conforming to the SQL on FHIR {@code SQLQuery} or {@code + * SQLView} profile. Extracts the SQL text, dependency references, and (for {@code SQLQuery}) + * parameter declarations, and enforces the profile invariants common to both. * - *

    The SQLQuery profile requires: + *

    The two profiles are near-twins. Both require: * *

      - *
    • {@code Library.type} carrying a coding of {@code sql-query} from the SQL on FHIR library - * types code system. + *
    • {@code Library.type} carrying a coding from the SQL on FHIR library types code system - + * {@code sql-query} for a {@code SQLQuery}, {@code sql-view} for a {@code SQLView}. *
    • A {@code content} entry with content type starting with {@code application/sql} containing * Base64-encoded SQL text. *
    • Each {@code relatedArtifact} of type {@code depends-on}, with a label matching {@code - * ^[A-Za-z][A-Za-z0-9_]*$} and a {@code resource} canonical URL pointing at the referenced - * ViewDefinition. - *
    • Each {@code parameter} declared with {@code use = in} and a name and type. + * ^[A-Za-z][A-Za-z0-9_]*$} and a {@code resource} reference pointing at the referenced {@code + * ViewDefinition} or {@code SQLView}. *
    * + *

    They differ in only one rule: a {@code SQLQuery} may declare input {@code parameter}s (each + * {@code use = in}), whereas a {@code SQLView} SHALL NOT declare any parameter. + * + * @author John Grimes * @see SQLQuery + * @see SQLView */ @Component -public class SqlQueryLibraryParser { +public class SqlLibraryParser { private static final String SQL_CONTENT_TYPE_PREFIX = "application/sql"; - /** Code system identifying the SQLQuery Library profile. */ + /** Code system identifying the SQL on FHIR Library profiles. */ public static final String LIBRARY_TYPE_SYSTEM = "https://sql-on-fhir.org/ig/CodeSystem/LibraryTypesCodes"; - /** {@link #LIBRARY_TYPE_SYSTEM} code identifying a SQLQuery Library. */ - public static final String LIBRARY_TYPE_CODE = "sql-query"; + /** {@link #LIBRARY_TYPE_SYSTEM} code identifying a {@code SQLQuery} Library. */ + public static final String SQL_QUERY_TYPE_CODE = "sql-query"; + + /** {@link #LIBRARY_TYPE_SYSTEM} code identifying a {@code SQLView} Library. */ + public static final String SQL_VIEW_TYPE_CODE = "sql-view"; private static final Pattern LABEL_PATTERN = Pattern.compile("^[A-Za-z]\\w*$"); /** - * Parses a Library resource into a {@link ParsedSqlQuery}. + * Parses a Library resource into a {@link ParsedSqlQuery}, accepting either a {@code SQLQuery} or + * a {@code SQLView}. * * @param library the Library resource to parse - * @return the parsed SQL query containing SQL text, view references, and parameter declarations - * @throws InvalidRequestException if the Library does not conform to the SQLQuery profile + * @return the parsed query carrying SQL text, dependency references, parameter declarations, and + * the resolved library type code + * @throws InvalidRequestException if the Library does not conform to either profile */ @Nonnull public ParsedSqlQuery parse(@Nonnull final Library library) { - validateLibraryType(library); - final String sql = extractSql(library); + final String typeCode = resolveLibraryTypeCode(library); + final boolean isView = SQL_VIEW_TYPE_CODE.equals(typeCode); + final String sql = extractSql(library, typeCode); final List viewReferences = extractViewReferences(library); - final List parameters = extractParameters(library); - return new ParsedSqlQuery(sql, viewReferences, parameters); + final List parameters = extractParameters(library, isView); + return new ParsedSqlQuery(sql, viewReferences, parameters, typeCode); } /** - * Verifies that the Library carries the SQLQuery profile's type coding. The check accepts any - * coding with the expected system and code, regardless of additional codings, so that authors can - * layer their own classifications without breaking conformance. + * Resolves the SQL on FHIR library type code carried by {@code Library.type}. The check accepts a + * coding with the expected system and either the {@code sql-query} or {@code sql-view} code, + * regardless of additional codings, so that authors can layer their own classifications without + * breaking conformance. + * + * @return the matched type code ({@code sql-query} or {@code sql-view}) + * @throws InvalidRequestException if no recognised SQL on FHIR coding is present */ - private void validateLibraryType(@Nonnull final Library library) { + @Nonnull + private String resolveLibraryTypeCode(@Nonnull final Library library) { final CodeableConcept type = library.getType(); if (type == null || type.isEmpty()) { throw new InvalidRequestException( - "SQLQuery Library must declare Library.type with the SQLQuery coding (" + "SQL on FHIR Library must declare Library.type with a coding from " + LIBRARY_TYPE_SYSTEM - + "#" - + LIBRARY_TYPE_CODE + + " (" + + SQL_QUERY_TYPE_CODE + + " or " + + SQL_VIEW_TYPE_CODE + ")"); } for (final Coding coding : type.getCoding()) { - if (LIBRARY_TYPE_SYSTEM.equals(coding.getSystem()) - && LIBRARY_TYPE_CODE.equals(coding.getCode())) { - return; + if (LIBRARY_TYPE_SYSTEM.equals(coding.getSystem())) { + final String code = coding.getCode(); + if (SQL_QUERY_TYPE_CODE.equals(code) || SQL_VIEW_TYPE_CODE.equals(code)) { + return code; + } } } throw new InvalidRequestException( - "SQLQuery Library.type must include a coding with system " + "SQL on FHIR Library.type must include a coding with system " + LIBRARY_TYPE_SYSTEM + " and code " - + LIBRARY_TYPE_CODE); + + SQL_QUERY_TYPE_CODE + + " or " + + SQL_VIEW_TYPE_CODE); } /** * Extracts the SQL text from the Library's content entries. * * @param library the Library resource + * @param typeCode the resolved library type code, used in the error message * @return the decoded SQL text * @throws InvalidRequestException if no SQL content is found or the content is invalid */ @Nonnull - private String extractSql(@Nonnull final Library library) { + private String extractSql(@Nonnull final Library library, @Nonnull final String typeCode) { for (final Attachment attachment : library.getContent()) { final String contentType = attachment.getContentType(); if (contentType != null && contentType.startsWith(SQL_CONTENT_TYPE_PREFIX)) { final byte[] data = attachment.getData(); if (data == null || data.length == 0) { throw new InvalidRequestException( - "SQLQuery Library has an application/sql content entry with no data"); + "SQL on FHIR Library has an application/sql content entry with no data"); } // The data is Base64-encoded in the FHIR resource. HAPI decodes it automatically when // using getData(), so we can use it directly. @@ -134,15 +159,17 @@ private String extractSql(@Nonnull final Library library) { } } throw new InvalidRequestException( - "SQLQuery Library must contain a content entry with content type application/sql"); + "A " + + typeCode + + " Library must contain a content entry with content type application/sql"); } /** - * Extracts ViewDefinition references from the Library's related artifacts, enforcing that each - * artifact is of type {@code depends-on} with a label matching the SQLQuery profile pattern. + * Extracts dependency references from the Library's related artifacts, enforcing that each + * artifact is of type {@code depends-on} with a label matching the SQL on FHIR profile pattern. * * @param library the Library resource - * @return the list of view artifact references + * @return the list of dependency references */ @Nonnull private List extractViewReferences(@Nonnull final Library library) { @@ -150,7 +177,7 @@ private List extractViewReferences(@Nonnull final Library for (final RelatedArtifact artifact : library.getRelatedArtifact()) { if (artifact.getType() != RelatedArtifactType.DEPENDSON) { throw new InvalidRequestException( - "SQLQuery Library relatedArtifact must have type 'depends-on', but found '" + "SQL on FHIR Library relatedArtifact must have type 'depends-on', but found '" + (artifact.getType() == null ? "null" : artifact.getType().toCode()) + "'"); } @@ -158,18 +185,26 @@ private List extractViewReferences(@Nonnull final Library final String resource = artifact.getResource(); if (label == null || label.isBlank()) { throw new InvalidRequestException( - "Each relatedArtifact in the SQLQuery Library must have a label"); + "Each relatedArtifact in the SQL on FHIR Library must have a label"); } if (!LABEL_PATTERN.matcher(label).matches()) { throw new InvalidRequestException( - "SQLQuery Library relatedArtifact label '" + "SQL on FHIR Library relatedArtifact label '" + label + "' does not match the required pattern " + LABEL_PATTERN.pattern()); } if (resource == null || resource.isBlank()) { throw new InvalidRequestException( - "Each relatedArtifact in the SQLQuery Library must have a resource reference"); + "Each relatedArtifact in the SQL on FHIR Library must have a resource reference"); + } + if (!CanonicalReference.isCanonical(resource)) { + throw new InvalidRequestException( + "SQL on FHIR Library relatedArtifact.resource '" + + resource + + "' is not an absolute canonical URL; a canonical URL (http://, https:// or urn:," + + " optionally suffixed with |version) is required to reference a ViewDefinition or" + + " SQLView"); } references.add(new ViewArtifactReference(label, resource)); } @@ -177,14 +212,27 @@ private List extractViewReferences(@Nonnull final Library } /** - * Extracts parameter declarations from the Library's parameter entries, enforcing that each - * declaration is an input ({@code use = in}) and carries both a name and a type. + * Extracts parameter declarations from the Library's parameter entries. A {@code SQLView} SHALL + * NOT declare any parameter and is rejected if it does. For a {@code SQLQuery}, each declaration + * must be an input ({@code use = in}) and carry both a name and a type. * * @param library the Library resource - * @return the list of parameter declarations + * @param isView whether the Library is a {@code SQLView} + * @return the list of parameter declarations (always empty for a {@code SQLView}) + * @throws InvalidRequestException if a {@code SQLView} declares any parameter, or a {@code + * SQLQuery} parameter is malformed */ @Nonnull - private List extractParameters(@Nonnull final Library library) { + private List extractParameters( + @Nonnull final Library library, final boolean isView) { + if (isView) { + if (!library.getParameter().isEmpty()) { + throw new InvalidRequestException( + "A " + SQL_VIEW_TYPE_CODE + " Library must not declare any parameter"); + } + return List.of(); + } + final List parameters = new ArrayList<>(); for (final ParameterDefinition param : library.getParameter()) { final String name = param.getName(); @@ -209,4 +257,14 @@ private List extractParameters(@Nonnull final Library l } return parameters; } + + /** + * Indicates whether the given library type code denotes a {@code SQLView}. + * + * @param typeCode the SQL on FHIR library type code + * @return {@code true} if the code is {@code sql-view} + */ + public static boolean isView(@Nullable final String typeCode) { + return SQL_VIEW_TYPE_CODE.equals(typeCode); + } } diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExecutionHelper.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExecutionHelper.java index 2b4d0906e4..b25ace4c17 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExecutionHelper.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExecutionHelper.java @@ -18,7 +18,6 @@ package au.csiro.pathling.operations.sqlquery; import au.csiro.pathling.library.io.source.QueryableDataSource; -import au.csiro.pathling.views.FhirView; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import jakarta.annotation.Nonnull; import jakarta.annotation.Nullable; @@ -33,17 +32,15 @@ import org.springframework.stereotype.Component; /** - * Orchestrates the {@code $sqlquery-run} operation by chaining the parser, view resolver, executor - * and result streamer. + * Orchestrates the {@code $sqlquery-run} operation by selecting the query Library, running it + * through the shared {@link SqlQueryPipeline}, and streaming the single result. + * + * @author John Grimes */ @Component public class SqlQueryExecutionHelper { - @Nonnull private final SqlQueryRequestParser requestParser; - - @Nonnull private final ViewResolver viewResolver; - - @Nonnull private final SqlQueryExecutor executor; + @Nonnull private final SqlQueryPipeline pipeline; @Nonnull private final SqlQueryResultStreamer streamer; @@ -54,30 +51,39 @@ public class SqlQueryExecutionHelper { /** * Constructs a new SqlQueryExecutionHelper. * - * @param requestParser parses raw HTTP inputs into a validated request - * @param viewResolver resolves view references to parsed FhirViews with auth checks - * @param executor validates and runs the SQL against Spark + * @param pipeline the shared SQL query pipeline (parse, resolve, validate, execute) * @param streamer streams the result dataset in the requested format * @param deltaLake the queryable data source backing FhirView execution * @param libraryReferenceResolver resolves a queryReference to a stored Library */ - @SuppressWarnings("java:S107") @Autowired public SqlQueryExecutionHelper( - @Nonnull final SqlQueryRequestParser requestParser, - @Nonnull final ViewResolver viewResolver, - @Nonnull final SqlQueryExecutor executor, + @Nonnull final SqlQueryPipeline pipeline, @Nonnull final SqlQueryResultStreamer streamer, @Nonnull final QueryableDataSource deltaLake, @Nonnull final LibraryReferenceResolver libraryReferenceResolver) { - this.requestParser = requestParser; - this.viewResolver = viewResolver; - this.executor = executor; + this.pipeline = pipeline; this.streamer = streamer; this.deltaLake = deltaLake; this.libraryReferenceResolver = libraryReferenceResolver; } + /** + * Rejects the unsupported {@code source} parameter (external data source). Pathling does not + * implement external data sources, so a supplied {@code source} value is rejected rather than + * silently ignored. This shared guard backs the {@code source} rejection at the system, type, and + * instance levels, over both POST and GET. + * + * @param source the {@code source} parameter value, if supplied + * @throws InvalidRequestException if {@code source} is present and non-blank + */ + public void rejectSourceParameter(@Nullable final String source) { + if (source != null && !source.isBlank()) { + throw new InvalidRequestException( + "The 'source' parameter (external data source) is not supported by this server."); + } + } + /** * Executes a {@code $sqlquery-run} request and streams results to the HTTP response. Exactly one * of {@code queryResource} and {@code queryReference} must be provided. @@ -110,20 +116,19 @@ public void executeSqlQuery( final IBaseResource library = selectLibrary(queryResource, queryReference); - final SqlQueryRequest request = - requestParser.parse(library, format, acceptHeader, includeHeader, limit, parameters); - - final Map resolvedViews = - viewResolver.resolve(request.getParsedQuery().getViewReferences()); + final PreparedSqlQuery prepared = + pipeline.prepare(library, format, acceptHeader, includeHeader, limit, parameters, Map.of()); - executor.execute( - request, - resolvedViews, + pipeline.execute( + prepared, deltaLake, requestId, result -> streamer.stream( - result, request.getOutputFormat(), request.isIncludeHeader(), response)); + result, + prepared.getRequest().getOutputFormat(), + prepared.getRequest().isIncludeHeader(), + response)); } /** diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExecutor.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExecutor.java index 54e430efc8..e0deffa942 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExecutor.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExecutor.java @@ -17,16 +17,13 @@ package au.csiro.pathling.operations.sqlquery; -import au.csiro.pathling.config.ServerConfiguration; -import au.csiro.pathling.config.SqlQueryConfiguration; import au.csiro.pathling.io.source.DataSource; -import au.csiro.pathling.views.FhirView; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import jakarta.annotation.Nonnull; +import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; import java.util.function.Consumer; -import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; import org.apache.spark.sql.Dataset; import org.apache.spark.sql.Row; @@ -35,9 +32,17 @@ import org.springframework.stereotype.Component; /** - * Executes the SQL of a {@link SqlQueryRequest} against the configured Spark session, owning the - * lifecycle of the request-scoped temporary views the query references. The only piece of the - * pipeline that touches Spark. + * Executes the SQL of a {@link SqlQueryRequest} against the configured Spark session, materialising + * the request's resolved dependency graph as request-scoped temporary views and owning their + * lifecycle. The only piece of the pipeline that touches Spark. + * + *

    Each node of the graph is materialised in topological order: a {@code ViewDefinition} leaf is + * executed as a view, and a {@code SQLView} node's SQL is rewritten against the temp views of its + * already-materialised children, validated, and run. The top-level SQL is then rewritten against + * its own direct dependencies' temp views and run. Every node's SQL is validated statically before + * execution and against its analysed plan during execution. + * + * @author John Grimes */ @Slf4j @Component @@ -49,116 +54,146 @@ public class SqlQueryExecutor { @Nonnull private final SqlValidator sqlValidator; - @Nonnull private final SqlQueryConfiguration sqlQueryConfig; - - @Nonnull private final SqlQueryWatchdog watchdog; - /** * Constructs a new SqlQueryExecutor. * * @param sparkSession the Spark session * @param viewRegistrationService manages temp-view registration / cleanup and SQL rewriting * @param sqlValidator validates the SQL before execution - * @param serverConfiguration the server configuration, used to resolve the resource limits - * applied to each query - * @param watchdog the watchdog used to schedule wall-clock timeouts and Spark job-group - * cancellation for each query */ @Autowired public SqlQueryExecutor( @Nonnull final SparkSession sparkSession, @Nonnull final ViewRegistrationService viewRegistrationService, - @Nonnull final SqlValidator sqlValidator, - @Nonnull final ServerConfiguration serverConfiguration, - @Nonnull final SqlQueryWatchdog watchdog) { + @Nonnull final SqlValidator sqlValidator) { this.sparkSession = sparkSession; this.viewRegistrationService = viewRegistrationService; this.sqlValidator = sqlValidator; - this.sqlQueryConfig = serverConfiguration.getSqlQuery(); - this.watchdog = watchdog; } /** - * Validates and executes the SQL, registering the resolved views under request-scoped temp view - * names for the duration of the call. The {@code consumer} is invoked with the result dataset - * before the temp views are dropped, so streaming and other terminal operations can complete - * before cleanup. + * Runs the static, read-only SQL validation for every node of the graph and the top-level query, + * each against its own declared labels, without touching Spark. Used both at export kick-off and + * before execution so malformed or disallowed SQL is caught early. + * + * @param request the parsed request + * @param graph the resolved dependency graph + */ + public void validateStatically( + @Nonnull final SqlQueryRequest request, @Nonnull final ResolvedDependencyGraph graph) { + for (final ResolvedDependency node : graph.getOrderedNodes()) { + if (node instanceof final ResolvedSqlView sqlView) { + sqlValidator.validate(sqlView.getSql(), sqlView.getChildKeysByLabel().keySet()); + } + } + sqlValidator.validate( + request.getParsedQuery().getSql(), graph.getTopLevelKeysByLabel().keySet()); + } + + /** + * Validates and executes the query, materialising the resolved dependency graph under + * request-scoped temp view names for the duration of the call. The {@code consumer} is invoked + * with the result dataset before the temp views are dropped, so streaming and other terminal + * operations can complete before cleanup. + * + *

    The only row limit applied is the caller's {@code _limit}, when they supply one. Execution + * runs under whatever Spark job group the caller established, so an asynchronous job's Spark + * stages remain attributed to it and a cancellation of that group reaches the work in flight. * * @param request the parsed and validated request - * @param resolvedViews the views referenced by the SQL, keyed by table label + * @param graph the resolved dependency graph the SQL references * @param dataSource the data source backing FhirView execution * @param requestId the HAPI per-request id used to namespace temp view names * @param consumer terminal consumer of the result dataset */ public void execute( @Nonnull final SqlQueryRequest request, - @Nonnull final Map resolvedViews, + @Nonnull final ResolvedDependencyGraph graph, @Nonnull final DataSource dataSource, @Nonnull final String requestId, @Nonnull final Consumer> consumer) { - final Set declaredLabels = - request.getParsedQuery().getViewReferences().stream() - .map(ViewArtifactReference::getLabel) - .collect(Collectors.toUnmodifiableSet()); - sqlValidator.validate(request.getParsedQuery().getSql(), declaredLabels); + validateStatically(request, graph); - final String jobGroupId = "sqlquery-" + requestId; - sparkSession - .sparkContext() - .setJobGroup(jobGroupId, "$sqlquery-run " + requestId, /* interruptOnCancel= */ true); - - Map registeredViews = Map.of(); - final SqlQueryWatchdog.Watch watch = watchdog.start(jobGroupId); + final Map registeredByKey = new LinkedHashMap<>(); try { - registeredViews = viewRegistrationService.registerViews(resolvedViews, dataSource, requestId); + for (final ResolvedDependency node : graph.getOrderedNodes()) { + materialiseNode(node, dataSource, requestId, registeredByKey); + } + final Map topLevelViews = + resolveLabelToViewName(graph.getTopLevelKeysByLabel(), registeredByKey); final String rewrittenSql = - viewRegistrationService.rewriteSql(request.getParsedQuery().getSql(), registeredViews); + viewRegistrationService.rewriteSql(request.getParsedQuery().getSql(), topLevelViews); Dataset result = runSql(rewrittenSql, request.getParameterBindings()); sqlValidator.validateAnalyzed( - result.queryExecution().analyzed(), Set.copyOf(registeredViews.values())); + result.queryExecution().analyzed(), Set.copyOf(topLevelViews.values())); - result = result.limit(effectiveLimit(request.getLimit(), requestId)); + final Integer callerLimit = request.getLimit(); + if (callerLimit != null) { + result = result.limit(callerLimit); + } consumer.accept(result); - } catch (final RuntimeException e) { - if (watch.timedOut()) { - throw new InvalidRequestException( - "Query exceeded the configured timeout of " - + sqlQueryConfig.getTimeoutSeconds() - + " seconds."); - } - throw e; } finally { - watch.complete(); - sparkSession.sparkContext().clearJobGroup(); - viewRegistrationService.dropViews(registeredViews.values()); + viewRegistrationService.dropViews(registeredByKey.values()); } } /** - * Resolves the row limit applied to the result dataset. The configured server cap is always - * applied; when the caller supplies a {@code _limit}, the lower of the two values wins. The - * server cap is clamped to {@link Integer#MAX_VALUE} so that it can be passed to Spark's {@code - * Dataset.limit(int)} API. + * Materialises a single graph node as a request-scoped temp view, recording its name by canonical + * key. A {@code SQLView} node's analysed plan is validated against its own children's temp views + * before registration, so it cannot reach an unauthorised data source. */ - int effectiveLimit(final Integer callerLimit, @Nonnull final String requestId) { - final int cap = (int) Math.min(sqlQueryConfig.getMaxRows(), Integer.MAX_VALUE); - if (callerLimit == null) { - return cap; + private void materialiseNode( + @Nonnull final ResolvedDependency node, + @Nonnull final DataSource dataSource, + @Nonnull final String requestId, + @Nonnull final Map registeredByKey) { + final Dataset dataset; + if (node instanceof final ResolvedViewDefinition viewDefinition) { + dataset = viewRegistrationService.buildViewDefinition(viewDefinition.getView(), dataSource); + } else if (node instanceof final ResolvedSqlView sqlView) { + dataset = viewRegistrationService.buildSqlView(sqlView, registeredByKey); + final Set childViewNames = + Set.copyOf( + resolveLabelToViewName(sqlView.getChildKeysByLabel(), registeredByKey).values()); + sqlValidator.validateAnalyzed(dataset.queryExecution().analyzed(), childViewNames); + } else { + throw new InvalidRequestException( + "Unsupported dependency node type: " + node.getClass().getSimpleName()); } - if (callerLimit > cap) { - log.info( - "Caller-supplied _limit of {} clamped to server cap of {} for request {}.", - callerLimit, - cap, - requestId); - return cap; + final String tempViewName = + viewRegistrationService.registerDataset(node.getCanonicalKey(), dataset, requestId); + registeredByKey.put(node.getCanonicalKey(), tempViewName); + log.debug( + "Materialised temp view '{}' for dependency '{}'", tempViewName, node.getCanonicalKey()); + } + + /** + * Maps a node's local {@code label -> child canonical key} to {@code label -> temp view name}, + * resolving each child key against the views materialised so far. + */ + @Nonnull + private static Map resolveLabelToViewName( + @Nonnull final Map keysByLabel, + @Nonnull final Map registeredByKey) { + final Map labelToViewName = new LinkedHashMap<>(); + for (final Map.Entry entry : keysByLabel.entrySet()) { + final String viewName = registeredByKey.get(entry.getValue()); + if (viewName == null) { + throw new IllegalStateException( + "Dependency '" + + entry.getValue() + + "' for label '" + + entry.getKey() + + "' was not materialised before it was referenced"); + } + labelToViewName.put(entry.getKey(), viewName); } - return callerLimit; + return labelToViewName; } @Nonnull diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportExecutor.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportExecutor.java new file mode 100644 index 0000000000..f107e56cc8 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportExecutor.java @@ -0,0 +1,133 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import au.csiro.pathling.library.io.source.QueryableDataSource; +import au.csiro.pathling.operations.export.ExportDataSourceBuilder; +import au.csiro.pathling.operations.export.ExportFileWriter; +import au.csiro.pathling.operations.export.ExportManifestOutput; +import au.csiro.pathling.operations.view.ViewExportFormat; +import jakarta.annotation.Nonnull; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.hadoop.fs.Path; +import org.apache.spark.sql.Dataset; +import org.apache.spark.sql.Row; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Component; + +/** + * Runs each query of a {@code $sqlquery-export} request via the shared {@link SqlQueryPipeline} and + * writes the result of each to files, producing one output per query. Reuses the shared {@link + * ExportDataSourceBuilder} (filtering) and {@link ExportFileWriter} (job directory and file + * writing) that back {@code $viewdefinition-export}. + * + *

    Execution is all-or-nothing: if any query fails, the exception propagates and the whole export + * fails, so no completion manifest is produced. + * + * @author John Grimes + */ +@Component +public class SqlQueryExportExecutor { + + @Nonnull private final SqlQueryPipeline pipeline; + + @Nonnull private final QueryableDataSource deltaLake; + + @Nonnull private final ExportDataSourceBuilder dataSourceBuilder; + + @Nonnull private final ExportFileWriter fileWriter; + + /** + * Constructs a new SqlQueryExportExecutor. + * + * @param pipeline the shared SQL query pipeline (execution) + * @param deltaLake the queryable data source backing FhirView execution + * @param dataSourceBuilder the shared export data-source builder (applies filters) + * @param fileWriter the shared export file writer (job directory and file writing) + */ + @Autowired + public SqlQueryExportExecutor( + @Nonnull final SqlQueryPipeline pipeline, + @Nonnull final QueryableDataSource deltaLake, + @Nonnull final ExportDataSourceBuilder dataSourceBuilder, + @Nonnull final ExportFileWriter fileWriter) { + this.pipeline = pipeline; + this.deltaLake = deltaLake; + this.dataSourceBuilder = dataSourceBuilder; + this.fileWriter = fileWriter; + } + + /** + * Executes the export request and writes the results to files, one output per query. + * + * @param request the export request + * @param jobId the job id for this export + * @return the outputs, one per query, in order + */ + @Nonnull + public List execute( + @Nonnull final SqlQueryExportRequest request, @Nonnull final String jobId) { + + final Path jobDirPath = fileWriter.createJobDirectory(jobId); + final QueryableDataSource dataSource = + dataSourceBuilder.build(deltaLake, request.since(), request.patientIds()); + final List outputs = new ArrayList<>(); + final Set usedNames = new HashSet<>(); + + for (int i = 0; i < request.queries().size(); i++) { + final QueryInput query = request.queries().get(i); + final String outputName = fileWriter.uniqueName(query.getEffectiveName(i), usedNames); + usedNames.add(outputName); + + // Namespace the request-scoped temp views per query within the job. + final String requestId = jobId + "-" + i; + final AtomicReference> fileUrls = new AtomicReference<>(List.of()); + pipeline.execute( + query.preparedQuery(), + dataSource, + requestId, + result -> + fileUrls.set( + writeOutput( + result, outputName, request.format(), request.includeHeader(), jobDirPath))); + + outputs.add(new ExportManifestOutput(outputName, fileUrls.get())); + } + + return outputs; + } + + /** Writes the query result in the requested format via the shared file writer. */ + @Nonnull + private List writeOutput( + @Nonnull final Dataset result, + @Nonnull final String name, + @Nonnull final ViewExportFormat format, + final boolean includeHeader, + @Nonnull final Path jobDirPath) { + return switch (format) { + case NDJSON -> fileWriter.writeNdjson(result, name, jobDirPath); + case CSV -> fileWriter.writeCsv(result, name, includeHeader, jobDirPath); + case PARQUET -> fileWriter.writeParquet(result, name, jobDirPath); + }; + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportProvider.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportProvider.java new file mode 100644 index 0000000000..6977379c50 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportProvider.java @@ -0,0 +1,125 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import au.csiro.pathling.async.AsyncPattern; +import au.csiro.pathling.async.AsyncSupported; +import au.csiro.pathling.async.PreAsyncValidation; +import au.csiro.pathling.security.OperationAccess; +import ca.uhn.fhir.rest.annotation.Operation; +import ca.uhn.fhir.rest.annotation.OperationParam; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.util.Collections; +import java.util.List; +import lombok.extern.slf4j.Slf4j; +import org.hl7.fhir.r4.model.BooleanType; +import org.hl7.fhir.r4.model.InstantType; +import org.hl7.fhir.r4.model.Parameters; +import org.hl7.fhir.r4.model.Reference; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Component; + +/** + * Provider for the system-level {@code $sqlquery-export} operation from the SQL on FHIR + * specification: the asynchronous counterpart to {@code $sqlquery-run}. Runs one or more SQL + * queries against materialised ViewDefinition tables in the background and exports each result to + * downloadable files. The level-agnostic machinery is shared via {@link SqlQueryExportSupport}. + * + * @author John Grimes + * @see $sqlquery-export + * @see SqlQueryInstanceExportProvider for the type-level and instance-level operations + */ +@Slf4j +@Component +public class SqlQueryExportProvider implements PreAsyncValidation { + + @Nonnull private final SqlQueryExportRequestParser requestParser; + + @Nonnull private final SqlQueryExportSupport support; + + /** + * Constructs a new SqlQueryExportProvider. + * + * @param requestParser parses and validates the kick-off request + * @param support the shared export machinery (job resolution, execution, manifest, cache key) + */ + @Autowired + public SqlQueryExportProvider( + @Nonnull final SqlQueryExportRequestParser requestParser, + @Nonnull final SqlQueryExportSupport support) { + this.requestParser = requestParser; + this.support = support; + } + + /** + * Handles the {@code $sqlquery-export} operation at the system level. + * + * @param clientTrackingId optional client-provided tracking identifier + * @param format the output format (ndjson, csv, parquet) + * @param includeHeader whether to include headers in CSV output + * @param patientIds patient ids to filter by + * @param groupIds group ids to filter by + * @param since filter resources modified after this timestamp + * @param source the unsupported external data source parameter, rejected when supplied + * @param requestDetails the request details + * @return the completion manifest, or null if cancelled + */ + @SuppressWarnings({"unused", "java:S107"}) + @Operation(name = "$sqlquery-export", idempotent = true) + @OperationAccess("sqlquery-export") + @AsyncSupported(pattern = AsyncPattern.STANDARD_ASYNC_PATTERN) + @Nullable + public Parameters export( + @Nullable @OperationParam(name = "clientTrackingId") final String clientTrackingId, + @Nullable @OperationParam(name = "_format") final String format, + @Nullable @OperationParam(name = "header") final BooleanType includeHeader, + @Nullable @OperationParam(name = "patient") final List patientIds, + @Nullable @OperationParam(name = "group") final List groupIds, + @Nullable @OperationParam(name = "_since") final InstantType since, + @Nullable @OperationParam(name = "source") final String source, + @Nonnull final ServletRequestDetails requestDetails) { + return support.runExport(requestDetails, this); + } + + @Override + @Nonnull + public PreAsyncValidationResult preAsyncValidate( + @Nonnull final ServletRequestDetails servletRequestDetails, @Nonnull final Object[] params) + throws InvalidRequestException { + final SqlQueryExportRequest request = + requestParser.parse( + servletRequestDetails, + /* boundLibrary= */ null, + support.stringParam(servletRequestDetails, "_format"), + support.headerParam(servletRequestDetails), + support.stringParam(servletRequestDetails, "clientTrackingId"), + support.collectPatientIds(servletRequestDetails), + support.sinceParam(servletRequestDetails), + support.stringParam(servletRequestDetails, "source")); + return new PreAsyncValidationResult<>(request, Collections.emptyList()); + } + + @Override + @Nonnull + public String computeCacheKeyComponent(@Nonnull final SqlQueryExportRequest request) { + return support.computeCacheKeyComponent(request); + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportRequest.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportRequest.java new file mode 100644 index 0000000000..0ec130d74c --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportRequest.java @@ -0,0 +1,51 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import au.csiro.pathling.operations.view.ViewExportFormat; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.util.List; +import java.util.Set; +import org.hl7.fhir.r4.model.InstantType; + +/** + * The parsed and validated kick-off request for a {@code $sqlquery-export} operation, produced by + * {@link SqlQueryExportRequestParser#parse} and carried to background execution via the job. Each + * {@link QueryInput} is already parsed, parameter bound, and view resolved; view sources are folded + * into the per-query resolved views and produce no outputs of their own. + * + * @param originalRequest the original request URL + * @param serverBaseUrl the FHIR server base URL (used for constructing result/download URLs) + * @param queries the ordered list of queries; one output per query + * @param clientTrackingId optional client-provided tracking identifier, echoed when present + * @param format the output format (NDJSON, CSV, or Parquet) + * @param includeHeader whether to include a header row in CSV output + * @param patientIds patient ids to filter by (from {@code patient} and {@code group} parameters) + * @param since filter resources modified after this timestamp + * @author John Grimes + */ +public record SqlQueryExportRequest( + @Nonnull String originalRequest, + @Nonnull String serverBaseUrl, + @Nonnull List queries, + @Nullable String clientTrackingId, + @Nonnull ViewExportFormat format, + boolean includeHeader, + @Nonnull Set patientIds, + @Nullable InstantType since) {} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportRequestParser.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportRequestParser.java new file mode 100644 index 0000000000..07ab5360f6 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportRequestParser.java @@ -0,0 +1,363 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import au.csiro.pathling.config.ServerConfiguration; +import au.csiro.pathling.encoders.ViewDefinitionResource; +import au.csiro.pathling.errors.UnsupportedFhirPathFeatureError; +import au.csiro.pathling.library.io.source.QueryableDataSource; +import au.csiro.pathling.operations.view.ViewExecutionHelper; +import au.csiro.pathling.operations.view.ViewExportFormat; +import au.csiro.pathling.security.PathlingAuthority; +import au.csiro.pathling.security.ResourceAccess.AccessType; +import au.csiro.pathling.security.SecurityAspect; +import au.csiro.pathling.views.FhirView; +import au.csiro.pathling.views.FhirViewExecutor; +import au.csiro.pathling.views.ViewDefinitionGson; +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; +import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; +import com.google.gson.Gson; +import com.google.gson.JsonSyntaxException; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import jakarta.validation.ConstraintViolationException; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.r4.model.BooleanType; +import org.hl7.fhir.r4.model.InstantType; +import org.hl7.fhir.r4.model.Library; +import org.hl7.fhir.r4.model.Parameters; +import org.hl7.fhir.r4.model.Parameters.ParametersParameterComponent; +import org.hl7.fhir.r4.model.Reference; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Component; + +/** + * Parses the raw {@code $sqlquery-export} kick-off inputs into a validated {@link + * SqlQueryExportRequest}. Performs every check that does not require executing the queries: source + * rejection, strict {@code _format} parsing, per-{@code query} and per-{@code view} exclusivity, + * query Library resolution, request-supplied view resolution and semantic validation, parameter + * binding, and static SQL validation. Each query is prepared (parsed, bound, view-resolved) via the + * shared {@link SqlQueryPipeline} so that the export and run operations share identical semantics. + * + * @author John Grimes + */ +@Component +public class SqlQueryExportRequestParser { + + @Nonnull private final SqlQueryPipeline pipeline; + + @Nonnull private final LibraryReferenceResolver libraryReferenceResolver; + + @Nonnull private final ViewExecutionHelper viewExecutionHelper; + + @Nonnull private final FhirContext fhirContext; + + @Nonnull private final ServerConfiguration serverConfiguration; + + @Nonnull private final QueryableDataSource deltaLake; + + @Nonnull private final Gson gson; + + /** + * Constructs a new SqlQueryExportRequestParser. + * + * @param pipeline the shared SQL query pipeline (prepare and static validation) + * @param libraryReferenceResolver resolves a {@code queryReference} to a stored SQLQuery Library + * @param viewExecutionHelper resolves a {@code view.viewReference} to a stored ViewDefinition, + * reusing the per-part exclusivity and reference-resolution semantics of the view operations + * @param fhirContext the FHIR context, used to serialise supplied ViewDefinitions for parsing + * @param serverConfiguration the server configuration (auth toggle and query config) + * @param deltaLake the data source used to semantically validate supplied ViewDefinitions + */ + @SuppressWarnings("java:S107") + @Autowired + public SqlQueryExportRequestParser( + @Nonnull final SqlQueryPipeline pipeline, + @Nonnull final LibraryReferenceResolver libraryReferenceResolver, + @Nonnull final ViewExecutionHelper viewExecutionHelper, + @Nonnull final FhirContext fhirContext, + @Nonnull final ServerConfiguration serverConfiguration, + @Nonnull final QueryableDataSource deltaLake) { + this.pipeline = pipeline; + this.libraryReferenceResolver = libraryReferenceResolver; + this.viewExecutionHelper = viewExecutionHelper; + this.fhirContext = fhirContext; + this.serverConfiguration = serverConfiguration; + this.deltaLake = deltaLake; + this.gson = ViewDefinitionGson.create(); + } + + /** + * Parses and validates the kick-off request. + * + * @param requestDetails the servlet request details (for the raw Parameters body and URLs) + * @param boundLibrary the bound Library at instance level, or null at system/type level + * @param format the explicit {@code _format} parameter, if any + * @param includeHeader whether to include a CSV header row; {@code null} defaults to {@code true} + * @param clientTrackingId optional client-provided tracking identifier + * @param patientIds patient ids to filter by, resolved from {@code patient} and {@code group} + * @param since the {@code _since} filter, if any + * @param source the unsupported {@code source} parameter, rejected when supplied + * @return the validated request + * @throws InvalidRequestException (400) for statically detectable structural failures + */ + @Nonnull + @SuppressWarnings("java:S107") + public SqlQueryExportRequest parse( + @Nonnull final ServletRequestDetails requestDetails, + @Nullable final IBaseResource boundLibrary, + @Nullable final String format, + @Nullable final BooleanType includeHeader, + @Nullable final String clientTrackingId, + @Nonnull final Set patientIds, + @Nullable final InstantType since, + @Nullable final String source) { + + // Reject the unsupported source parameter synchronously, before any other work. + if (source != null && !source.isBlank()) { + throw new InvalidRequestException( + "The 'source' parameter (external data source) is not supported by this server."); + } + + // Parse the explicit _format strictly, so an unsupported value (e.g. json, fhir) is rejected at + // kick-off regardless of the query parameters. + final ViewExportFormat exportFormat = ViewExportFormat.fromString(format); + + final Parameters parameters = extractParameters(requestDetails); + + // Resolve request-supplied views (system/type level only); the bound-Library instance level + // resolves its views from server storage. + final Map suppliedViews = + boundLibrary == null ? resolveSuppliedViews(parameters) : Map.of(); + + final List queries = new ArrayList<>(); + if (boundLibrary != null) { + // Instance level: the bound Library is the single query source; the query parameter does not + // apply and per-query parameter binding is not offered. + queries.add(prepareQuery(null, boundLibrary, null, suppliedViews)); + } else { + for (final RawQuery rawQuery : extractQueries(parameters)) { + final IBaseResource library = + selectLibrary(rawQuery.queryResource(), rawQuery.queryReference()); + queries.add(prepareQuery(rawQuery.name(), library, rawQuery.parameters(), suppliedViews)); + } + if (queries.isEmpty()) { + throw new InvalidRequestException( + "At least one 'query' parameter is required at the system and type levels."); + } + } + + final boolean header = includeHeader == null || includeHeader.booleanValue(); + + return new SqlQueryExportRequest( + requestDetails.getCompleteUrl(), + requestDetails.getFhirServerBase(), + queries, + clientTrackingId, + exportFormat, + header, + patientIds, + since); + } + + /** + * Prepares a single query: resolves its Library name fallback, then parses, binds parameters, and + * resolves views via the shared pipeline, and statically validates the SQL. + */ + @Nonnull + private QueryInput prepareQuery( + @Nullable final String name, + @Nonnull final IBaseResource library, + @Nullable final Parameters parameters, + @Nonnull final Map suppliedViews) { + final PreparedSqlQuery prepared = + pipeline.prepare(library, null, null, null, null, parameters, suppliedViews); + pipeline.validateStatically(prepared); + return new QueryInput(name, libraryName(library), prepared); + } + + /** Returns the SQLQuery Library's {@code name} element, or null when not a named Library. */ + @Nullable + private static String libraryName(@Nonnull final IBaseResource library) { + return library instanceof final Library lib && lib.hasName() ? lib.getName() : null; + } + + /** + * Enforces the "exactly one of queryResource / queryReference" contract and resolves the Library. + */ + @Nonnull + private IBaseResource selectLibrary( + @Nullable final IBaseResource queryResource, @Nullable final Reference queryReference) { + final boolean hasResource = queryResource != null; + final boolean hasReference = queryReference != null && !queryReference.isEmpty(); + + if (hasResource && hasReference) { + throw new InvalidRequestException( + "Each 'query' must supply exactly one of 'queryResource' and 'queryReference', not" + + " both."); + } + if (!hasResource && !hasReference) { + throw new InvalidRequestException( + "Each 'query' must supply one of 'queryResource' or 'queryReference'."); + } + return hasResource ? queryResource : libraryReferenceResolver.resolve(queryReference); + } + + /** + * Resolves the {@code view} parts into a map keyed by the canonical url they satisfy, parsing + * inline views, reading referenced views, applying the per-resource READ check to stored views, + * and semantically validating each supplied view (a malformed view is a 400; a semantically + * invalid one a 422). A supplied view that carries no url is rejected with a 400, since it cannot + * satisfy a canonical dependency reference. + */ + @Nonnull + private Map resolveSuppliedViews(@Nonnull final Parameters parameters) { + final Map resolved = new LinkedHashMap<>(); + for (final ParametersParameterComponent param : parameters.getParameter()) { + if (!"view".equals(param.getName())) { + continue; + } + IBaseResource viewResource = null; + Reference viewReference = null; + for (final ParametersParameterComponent part : param.getPart()) { + if ("viewResource".equals(part.getName()) && part.getResource() != null) { + viewResource = part.getResource(); + } else if ("viewReference".equals(part.getName()) + && part.getValue() instanceof final Reference reference) { + viewReference = reference; + } + } + + // resolveViewInput enforces per-part exclusivity (400), presence (400), and reference + // resolution (404), and returns the resolved ViewDefinition resource. + final boolean inline = viewResource != null; + final IBaseResource resolvedResource = + viewExecutionHelper.resolveViewInput(viewResource, viewReference); + + // A supplied view satisfies a dependency reference by its canonical url; one without a url + // can never match a canonical reference, so it is rejected up front rather than silently + // ignored, ahead of the heavier parse and semantic validation. + final String url = + resolvedResource instanceof final ViewDefinitionResource viewDefinition + ? viewDefinition.getUrl() + : null; + if (url == null || url.isBlank()) { + throw new InvalidRequestException( + "A supplied 'view' must carry a url to satisfy a canonical dependency reference, but" + + " the supplied view has none"); + } + + final FhirView view = parseViewDefinition(resolvedResource); + + // A stored ViewDefinition is subject to the per-resource READ check; an inline view carries + // its own content and is authorised as the request payload. + if (!inline && serverConfiguration.getAuth().isEnabled()) { + SecurityAspect.checkHasAuthority( + PathlingAuthority.resourceAccess(AccessType.READ, view.getResource())); + } + + validateViewSemantically(view); + + resolved.put(url, view); + } + return resolved; + } + + /** Parses a ViewDefinition resource into a FhirView via JSON round-tripping. */ + @Nonnull + private FhirView parseViewDefinition(@Nonnull final IBaseResource viewResource) { + try { + final String viewJson = fhirContext.newJsonParser().encodeResourceToString(viewResource); + return gson.fromJson(viewJson, FhirView.class); + } catch (final JsonSyntaxException e) { + throw new InvalidRequestException("Invalid ViewDefinition: " + e.getMessage()); + } + } + + /** + * Semantically validates a supplied ViewDefinition by building its query plan, consistent with + * the view operations: a semantically invalid view is a 422, an unsupported expression a 400. + */ + private void validateViewSemantically(@Nonnull final FhirView view) { + try { + new FhirViewExecutor(fhirContext, deltaLake, serverConfiguration.getQuery()).buildQuery(view); + } catch (final ConstraintViolationException e) { + throw new UnprocessableEntityException("Invalid ViewDefinition: " + e.getMessage()); + } catch (final UnsupportedOperationException | UnsupportedFhirPathFeatureError e) { + throw new InvalidRequestException("Unsupported expression: " + e.getMessage()); + } + } + + /** Extracts the raw {@code query} parts from the request Parameters, preserving order. */ + @Nonnull + private List extractQueries(@Nonnull final Parameters parameters) { + final List queries = new ArrayList<>(); + for (final ParametersParameterComponent param : parameters.getParameter()) { + if (!"query".equals(param.getName())) { + continue; + } + String name = null; + IBaseResource queryResource = null; + Reference queryReference = null; + Parameters queryParameters = null; + for (final ParametersParameterComponent part : param.getPart()) { + switch (part.getName()) { + case "name" -> name = part.getValue() != null ? part.getValue().primitiveValue() : null; + case "queryResource" -> queryResource = part.getResource(); + case "queryReference" -> { + if (part.getValue() instanceof final Reference reference) { + queryReference = reference; + } + } + case "parameters" -> { + if (part.getResource() instanceof final Parameters params) { + queryParameters = params; + } + } + default -> { + // Ignore unrecognised parts. + } + } + } + queries.add(new RawQuery(name, queryResource, queryReference, queryParameters)); + } + return queries; + } + + /** Extracts the Parameters resource from the request body, or an empty one when absent. */ + @Nonnull + private static Parameters extractParameters(@Nonnull final ServletRequestDetails requestDetails) { + return requestDetails.getResource() instanceof final Parameters parameters + ? parameters + : new Parameters(); + } + + /** A raw, unresolved {@code query} part extracted from the request Parameters. */ + private record RawQuery( + @Nullable String name, + @Nullable IBaseResource queryResource, + @Nullable Reference queryReference, + @Nullable Parameters parameters) {} +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportSupport.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportSupport.java new file mode 100644 index 0000000000..fb6accd423 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportSupport.java @@ -0,0 +1,377 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import static au.csiro.pathling.security.SecurityAspect.getCurrentUserId; + +import au.csiro.pathling.async.AsyncJobContext; +import au.csiro.pathling.async.Job; +import au.csiro.pathling.async.JobRegistry; +import au.csiro.pathling.async.PreAsyncValidation; +import au.csiro.pathling.async.PreAsyncValidation.PreAsyncValidationResult; +import au.csiro.pathling.async.RequestTag; +import au.csiro.pathling.async.RequestTagFactory; +import au.csiro.pathling.config.ServerConfiguration; +import au.csiro.pathling.errors.AccessDeniedError; +import au.csiro.pathling.operations.bulkexport.ExportResult; +import au.csiro.pathling.operations.bulkexport.ExportResultRegistry; +import au.csiro.pathling.operations.compartment.GroupMemberService; +import au.csiro.pathling.operations.export.ExportFileWriter; +import au.csiro.pathling.operations.export.ExportManifest; +import au.csiro.pathling.operations.export.ExportManifestOutput; +import au.csiro.pathling.views.ViewDefinitionGson; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; +import com.google.gson.Gson; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.time.Instant; +import java.time.ZoneOffset; +import java.time.ZonedDateTime; +import java.time.format.DateTimeFormatter; +import java.util.HashSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; +import org.hl7.fhir.r4.model.BooleanType; +import org.hl7.fhir.r4.model.InstantType; +import org.hl7.fhir.r4.model.Parameters; +import org.hl7.fhir.r4.model.Reference; +import org.hl7.fhir.r4.model.Type; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.stereotype.Component; + +/** + * Shared machinery for the {@code $sqlquery-export} providers across the system, type, and instance + * levels: owning-job resolution, background execution, completion-manifest construction, the + * deterministic cache key, and extraction of the simple kick-off parameters from the request body. + * Both {@link SqlQueryExportProvider} (system) and {@link SqlQueryInstanceExportProvider} + * (type/instance) delegate here so the level-agnostic logic lives in one place. + * + * @author John Grimes + */ +@Component +public class SqlQueryExportSupport { + + @Nonnull private final SqlQueryExportExecutor executor; + + @Nonnull private final JobRegistry jobRegistry; + + @Nonnull private final RequestTagFactory requestTagFactory; + + @Nonnull private final ExportResultRegistry exportResultRegistry; + + @Nonnull private final ServerConfiguration serverConfiguration; + + @Nonnull private final GroupMemberService groupMemberService; + + @Nonnull private final ExportFileWriter fileWriter; + + @Nonnull private final Gson gson; + + /** + * Constructs a new SqlQueryExportSupport. + * + * @param executor runs the queries and writes the output files + * @param jobRegistry the async job registry + * @param requestTagFactory the request tag factory used for job deduplication + * @param exportResultRegistry the export result registry backing the {@code $result} endpoint + * @param serverConfiguration the server configuration + * @param groupMemberService resolves {@code group} references to member patient ids + * @param fileWriter the shared export file writer, used to clean up partial outputs on failure + */ + @SuppressWarnings("java:S107") + @Autowired + public SqlQueryExportSupport( + @Nonnull final SqlQueryExportExecutor executor, + @Nonnull final JobRegistry jobRegistry, + @Nonnull final RequestTagFactory requestTagFactory, + @Nonnull final ExportResultRegistry exportResultRegistry, + @Nonnull final ServerConfiguration serverConfiguration, + @Nonnull final GroupMemberService groupMemberService, + @Nonnull final ExportFileWriter fileWriter) { + this.executor = executor; + this.jobRegistry = jobRegistry; + this.requestTagFactory = requestTagFactory; + this.exportResultRegistry = exportResultRegistry; + this.serverConfiguration = serverConfiguration; + this.groupMemberService = groupMemberService; + this.fileWriter = fileWriter; + this.gson = ViewDefinitionGson.create(); + } + + /** + * Resolves the owning job, runs the export in the background, and builds the completion manifest. + * + * @param requestDetails the request details + * @param validation the provider's pre-async validation (used for the fallback job lookup) + * @return the completion manifest, or null if the job was cancelled + */ + @Nullable + public Parameters runExport( + @Nonnull final ServletRequestDetails requestDetails, + @Nonnull final PreAsyncValidation validation) { + final Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + + final Job ownJob = + resolveOwnJob(requestDetails, authentication, validation); + if (ownJob == null) { + throw new InvalidRequestException("Missing 'Prefer: respond-async' header value."); + } + + // Check that the user requesting the result is the same user that started the job. + final Optional currentUserId = getCurrentUserId(authentication); + if (currentUserId.isPresent() && !ownJob.getOwnerId().equals(currentUserId)) { + throw new AccessDeniedError( + "The requested result is not owned by the current user '%s'." + .formatted(currentUserId.orElse("null"))); + } + + final SqlQueryExportRequest exportRequest = ownJob.getPreAsyncValidationResult(); + if (ownJob.isCancelled()) { + return null; + } + + exportResultRegistry.put(ownJob.getId(), new ExportResult(ownJob.getOwnerId())); + + final List outputs; + try { + outputs = executor.execute(exportRequest, ownJob.getId()); + } catch (final RuntimeException e) { + // All-or-nothing: a failed query fails the whole export. Remove the result registration and + // delete any partial outputs so none are offered for download, then surface the failure. + exportResultRegistry.remove(ownJob.getId()); + fileWriter.deleteJobDirectory(ownJob.getId()); + throw e; + } + + // Set the Expires header on the completion response. + ownJob.setResponseModification( + httpServletResponse -> { + final String expiresValue = + ZonedDateTime.now(ZoneOffset.UTC) + .plusSeconds(serverConfiguration.getExport().getResultExpiry()) + .format(DateTimeFormatter.RFC_1123_DATE_TIME); + httpServletResponse.addHeader("Expires", expiresValue); + }); + + return new ExportManifest( + exportRequest.serverBaseUrl(), + ownJob.getId(), + exportRequest.clientTrackingId(), + exportRequest.format().getCode(), + ownJob.getStartTime(), + Instant.now(), + outputs) + .toParameters(); + } + + /** + * Resolves the job owning this request: the one set by the async aspect when running + * asynchronously, or - as a fallback when the async context is unavailable - the one looked up by + * recomputing the request tag. + */ + @Nullable + private Job resolveOwnJob( + @Nonnull final ServletRequestDetails requestDetails, + @Nullable final Authentication authentication, + @Nonnull final PreAsyncValidation validation) { + @SuppressWarnings("unchecked") + final Optional> contextJob = + AsyncJobContext.getCurrentJob().map(job -> (Job) job); + if (contextJob.isPresent()) { + return contextJob.get(); + } + + final PreAsyncValidationResult validationResult = + validation.preAsyncValidate(requestDetails, new Object[] {}); + final String operationCacheKey = + validation.computeCacheKeyComponent( + Objects.requireNonNull( + validationResult.result(), + "Validation result should not be null for a valid request")); + final RequestTag ownTag = + requestTagFactory.createTag(requestDetails, authentication, operationCacheKey); + return jobRegistry.get(ownTag); + } + + /** + * Computes the deterministic cache key component from the parsed request, so that identical + * kick-offs deduplicate to the same job. + * + * @param request the parsed request + * @return the cache key component + */ + @Nonnull + public String computeCacheKeyComponent(@Nonnull final SqlQueryExportRequest request) { + final StringBuilder key = new StringBuilder(); + + final String queriesJson = + request.queries().stream() + .map( + q -> + (q.name() != null ? q.name() : "") + + ":" + + q.preparedQuery().getRequest().getParsedQuery().getSql() + + ":" + + gson.toJson( + describeDependencyGraph(q.preparedQuery().getDependencyGraph())) + + ":" + + gson.toJson(q.preparedQuery().getRequest().getParameterBindings())) + .collect(Collectors.joining(",")); + key.append("queries=[").append(queriesJson).append("]"); + + if (request.clientTrackingId() != null) { + key.append("|clientTrackingId=").append(request.clientTrackingId()); + } + key.append("|format=").append(request.format()); + key.append("|header=").append(request.includeHeader()); + + if (!request.patientIds().isEmpty()) { + final String sortedPatientIds = + request.patientIds().stream().sorted().collect(Collectors.joining(",")); + key.append("|patientIds=[").append(sortedPatientIds).append("]"); + } + if (request.since() != null) { + key.append("|since=").append(request.since().getValueAsString()); + } + return key.toString(); + } + + /** + * Renders a resolved dependency graph as a deterministic, serialisable description for the cache + * key, so two kick-offs whose composed queries differ deduplicate to distinct jobs. Captures the + * top-level label-to-node mapping and, for each node, its canonical key and (for a SQLView) its + * SQL and child label mapping. + */ + @Nonnull + private static List describeDependencyGraph( + @Nonnull final ResolvedDependencyGraph graph) { + final List parts = new java.util.ArrayList<>(); + parts.add("top=" + graph.getTopLevelKeysByLabel()); + for (final ResolvedDependency node : graph.getOrderedNodes()) { + if (node instanceof final ResolvedSqlView sqlView) { + parts.add( + "view:" + + sqlView.getCanonicalKey() + + ":" + + sqlView.getSql() + + ":" + + sqlView.getChildKeysByLabel()); + } else { + parts.add("vd:" + node.getCanonicalKey()); + } + } + return parts; + } + + /** Collects patient ids from both the {@code patient} and {@code group} parameters. */ + @Nonnull + public Set collectPatientIds(@Nonnull final ServletRequestDetails requestDetails) { + final Set allPatientIds = new HashSet<>(); + for (final Parameters.ParametersParameterComponent param : + parametersOf(requestDetails).getParameter()) { + if ("patient".equals(param.getName())) { + final String id = stripResourcePrefix(referenceOrPrimitive(param.getValue())); + if (id != null && !id.isBlank()) { + allPatientIds.add(id); + } + } else if ("group".equals(param.getName())) { + final String groupId = stripResourcePrefix(referenceOrPrimitive(param.getValue())); + if (groupId != null && !groupId.isBlank()) { + allPatientIds.addAll(groupMemberService.extractPatientIdsFromGroup(groupId)); + } + } + } + return allPatientIds; + } + + /** + * Extracts a simple string-valued parameter from the request body. + * + * @param requestDetails the request details + * @param name the parameter name + * @return the primitive value, or null when absent + */ + @Nullable + public String stringParam( + @Nonnull final ServletRequestDetails requestDetails, @Nonnull final String name) { + for (final Parameters.ParametersParameterComponent param : + parametersOf(requestDetails).getParameter()) { + if (name.equals(param.getName()) && param.getValue() != null) { + return param.getValue().primitiveValue(); + } + } + return null; + } + + /** + * Extracts the {@code header} boolean parameter from the request body. + * + * @param requestDetails the request details + * @return the header flag, or null when absent + */ + @Nullable + public BooleanType headerParam(@Nonnull final ServletRequestDetails requestDetails) { + final String value = stringParam(requestDetails, "header"); + return value == null ? null : new BooleanType(value); + } + + /** + * Extracts the {@code _since} instant parameter from the request body. + * + * @param requestDetails the request details + * @return the since instant, or null when absent + */ + @Nullable + public InstantType sinceParam(@Nonnull final ServletRequestDetails requestDetails) { + final String value = stringParam(requestDetails, "_since"); + return value == null ? null : new InstantType(value); + } + + @Nullable + private static String referenceOrPrimitive(@Nullable final Type value) { + if (value == null) { + return null; + } + if (value instanceof final Reference reference) { + return reference.getReference(); + } + return value.primitiveValue(); + } + + @Nullable + private static String stripResourcePrefix(@Nullable final String reference) { + if (reference == null) { + return null; + } + final int slash = reference.lastIndexOf('/'); + return slash >= 0 ? reference.substring(slash + 1) : reference; + } + + @Nonnull + private static Parameters parametersOf(@Nonnull final ServletRequestDetails requestDetails) { + return requestDetails.getResource() instanceof final Parameters parameters + ? parameters + : new Parameters(); + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryInstanceExportProvider.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryInstanceExportProvider.java new file mode 100644 index 0000000000..7019544d54 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryInstanceExportProvider.java @@ -0,0 +1,197 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import au.csiro.pathling.async.AsyncPattern; +import au.csiro.pathling.async.AsyncSupported; +import au.csiro.pathling.async.PreAsyncValidation; +import au.csiro.pathling.security.OperationAccess; +import ca.uhn.fhir.rest.annotation.IdParam; +import ca.uhn.fhir.rest.annotation.Operation; +import ca.uhn.fhir.rest.annotation.OperationParam; +import ca.uhn.fhir.rest.server.IResourceProvider; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.util.Collections; +import java.util.List; +import lombok.extern.slf4j.Slf4j; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.r4.model.BooleanType; +import org.hl7.fhir.r4.model.IdType; +import org.hl7.fhir.r4.model.InstantType; +import org.hl7.fhir.r4.model.Library; +import org.hl7.fhir.r4.model.Parameters; +import org.hl7.fhir.r4.model.Reference; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Component; + +/** + * Provider for the type-level and instance-level {@code $sqlquery-export} operations for Library + * resources: + * + *

      + *
    • {@code POST /fhir/Library/$sqlquery-export} - type level, accepting the {@code query} and + * {@code view} parameters exactly as the system level does. + *
    • {@code POST /fhir/Library/[id]/$sqlquery-export} - instance level, exporting the bound + * Library as the single query source. + *
    + * + *

    Both reuse the shared {@link SqlQueryExportSupport} and {@link SqlQueryExportRequestParser}. + * + * @author John Grimes + * @see SqlQueryExportProvider for the system-level operation + */ +@Slf4j +@Component +public class SqlQueryInstanceExportProvider + implements IResourceProvider, PreAsyncValidation { + + @Nonnull private final SqlQueryExportRequestParser requestParser; + + @Nonnull private final SqlQueryExportSupport support; + + @Nonnull private final LibraryReferenceResolver libraryReferenceResolver; + + /** + * Constructs a new SqlQueryInstanceExportProvider. + * + * @param requestParser parses and validates the kick-off request + * @param support the shared export machinery + * @param libraryReferenceResolver resolves the bound Library id at instance level + */ + @Autowired + public SqlQueryInstanceExportProvider( + @Nonnull final SqlQueryExportRequestParser requestParser, + @Nonnull final SqlQueryExportSupport support, + @Nonnull final LibraryReferenceResolver libraryReferenceResolver) { + this.requestParser = requestParser; + this.support = support; + this.libraryReferenceResolver = libraryReferenceResolver; + } + + @Override + public Class getResourceType() { + return Library.class; + } + + /** + * Type-level {@code $sqlquery-export} operation, accepting the repeating {@code query} and {@code + * view} parameters exactly as the system-level operation does. + * + * @param clientTrackingId optional client-provided tracking identifier + * @param format the output format (ndjson, csv, parquet) + * @param includeHeader whether to include headers in CSV output + * @param patientIds patient ids to filter by + * @param groupIds group ids to filter by + * @param since filter resources modified after this timestamp + * @param source the unsupported external data source parameter, rejected when supplied + * @param requestDetails the request details + * @return the completion manifest, or null if cancelled + */ + @SuppressWarnings({"unused", "java:S107"}) + @Operation(name = "$sqlquery-export", idempotent = true) + @OperationAccess("sqlquery-export") + @AsyncSupported(pattern = AsyncPattern.STANDARD_ASYNC_PATTERN) + @Nullable + public Parameters exportType( + @Nullable @OperationParam(name = "clientTrackingId") final String clientTrackingId, + @Nullable @OperationParam(name = "_format") final String format, + @Nullable @OperationParam(name = "header") final BooleanType includeHeader, + @Nullable @OperationParam(name = "patient") final List patientIds, + @Nullable @OperationParam(name = "group") final List groupIds, + @Nullable @OperationParam(name = "_since") final InstantType since, + @Nullable @OperationParam(name = "source") final String source, + @Nonnull final ServletRequestDetails requestDetails) { + return support.runExport(requestDetails, this); + } + + /** + * Instance-level {@code $sqlquery-export} operation, exporting the bound Library as the single + * query source. + * + * @param libraryId the id of the stored Library to export + * @param format the output format (ndjson, csv, parquet) + * @param includeHeader whether to include headers in CSV output + * @param patientIds patient ids to filter by + * @param groupIds group ids to filter by + * @param since filter resources modified after this timestamp + * @param source the unsupported external data source parameter, rejected when supplied + * @param requestDetails the request details + * @return the completion manifest, or null if cancelled + */ + @SuppressWarnings({"unused", "java:S107"}) + @Operation(name = "$sqlquery-export", idempotent = true) + @OperationAccess("sqlquery-export") + @AsyncSupported(pattern = AsyncPattern.STANDARD_ASYNC_PATTERN) + @Nullable + public Parameters exportInstance( + @IdParam final IdType libraryId, + @Nullable @OperationParam(name = "_format") final String format, + @Nullable @OperationParam(name = "header") final BooleanType includeHeader, + @Nullable @OperationParam(name = "patient") final List patientIds, + @Nullable @OperationParam(name = "group") final List groupIds, + @Nullable @OperationParam(name = "_since") final InstantType since, + @Nullable @OperationParam(name = "source") final String source, + @Nonnull final ServletRequestDetails requestDetails) { + return support.runExport(requestDetails, this); + } + + @Override + @Nonnull + public PreAsyncValidationResult preAsyncValidate( + @Nonnull final ServletRequestDetails servletRequestDetails, @Nonnull final Object[] params) + throws InvalidRequestException { + // The bound Library at instance level is identified by the request id in the path; its absence + // marks a type-level invocation, which carries the query/view parameters instead. + final IBaseResource boundLibrary = resolveBoundLibrary(servletRequestDetails); + + final SqlQueryExportRequest request = + requestParser.parse( + servletRequestDetails, + boundLibrary, + support.stringParam(servletRequestDetails, "_format"), + support.headerParam(servletRequestDetails), + support.stringParam(servletRequestDetails, "clientTrackingId"), + support.collectPatientIds(servletRequestDetails), + support.sinceParam(servletRequestDetails), + support.stringParam(servletRequestDetails, "source")); + return new PreAsyncValidationResult<>(request, Collections.emptyList()); + } + + /** Resolves the bound Library at instance level, or returns null for a type-level invocation. */ + @Nullable + private IBaseResource resolveBoundLibrary( + @Nonnull final ServletRequestDetails servletRequestDetails) { + final IdType id = + servletRequestDetails.getId() == null + ? null + : new IdType(servletRequestDetails.getId().getValue()); + if (id == null || id.getIdPart() == null || id.getIdPart().isBlank()) { + return null; + } + return libraryReferenceResolver.resolve(new Reference("Library/" + id.getIdPart())); + } + + @Override + @Nonnull + public String computeCacheKeyComponent(@Nonnull final SqlQueryExportRequest request) { + return support.computeCacheKeyComponent(request); + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryInstanceRunProvider.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryInstanceRunProvider.java index a1b16a86df..8fc1344f0a 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryInstanceRunProvider.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryInstanceRunProvider.java @@ -49,6 +49,7 @@ * Library * * + * @author John Grimes * @see SQLQueryRun * @see SqlQueryRunProvider for system-level $sqlquery-run operation @@ -90,6 +91,7 @@ public Class getResourceType() { * @param includeHeader whether to include a header row in CSV output * @param limit the maximum number of rows to return * @param parameters runtime parameter bindings as a Parameters resource + * @param source the unsupported external data source parameter, rejected when supplied * @param requestDetails the servlet request details containing HTTP headers * @param response the HTTP response for streaming output */ @@ -103,9 +105,12 @@ public void runTypeLevel( @Nullable @OperationParam(name = "header") final BooleanType includeHeader, @Nullable @OperationParam(name = "_limit") final IntegerType limit, @Nullable @OperationParam(name = "parameters") final Parameters parameters, + @Nullable @OperationParam(name = "source") final String source, @Nonnull final ServletRequestDetails requestDetails, @Nullable final HttpServletResponse response) { + executionHelper.rejectSourceParameter(source); + final String acceptHeader = requestDetails.getServletRequest().getHeader("Accept"); executionHelper.executeSqlQuery( @@ -128,20 +133,25 @@ public void runTypeLevel( * @param includeHeader whether to include a header row in CSV output * @param limit the maximum number of rows to return * @param parameters runtime parameter bindings as a Parameters resource + * @param source the unsupported external data source parameter, rejected when supplied * @param requestDetails the servlet request details containing HTTP headers * @param response the HTTP response for streaming output */ @Operation(name = "$sqlquery-run", idempotent = true, manualResponse = true) @OperationAccess("sqlquery-run") + @SuppressWarnings("java:S107") public void runById( @IdParam final IdType libraryId, @Nullable @OperationParam(name = "_format") final String format, @Nullable @OperationParam(name = "header") final BooleanType includeHeader, @Nullable @OperationParam(name = "_limit") final IntegerType limit, @Nullable @OperationParam(name = "parameters") final Parameters parameters, + @Nullable @OperationParam(name = "source") final String source, @Nonnull final ServletRequestDetails requestDetails, @Nullable final HttpServletResponse response) { + executionHelper.rejectSourceParameter(source); + final IBaseResource libraryResource = libraryReferenceResolver.resolve(new Reference("Library/" + libraryId.getIdPart())); final String acceptHeader = requestDetails.getServletRequest().getHeader("Accept"); diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryOutputFormat.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryOutputFormat.java index de87733b3e..9cefb36a62 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryOutputFormat.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryOutputFormat.java @@ -17,6 +17,7 @@ package au.csiro.pathling.operations.sqlquery; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import jakarta.annotation.Nonnull; import jakarta.annotation.Nullable; import java.util.Arrays; @@ -28,6 +29,8 @@ * Output format options for the {@code $sqlquery-run} operation. Supports NDJSON, CSV, JSON, * Parquet, and FHIR ({@code Parameters} resource) formats as specified in the SQL on FHIR v2 * specification. + * + * @author John Grimes */ @Getter public enum SqlQueryOutputFormat { @@ -73,11 +76,42 @@ public static SqlQueryOutputFormat fromString(@Nullable final String format) { if (isNullOrBlank(format)) { return DEFAULT_FORMAT; } - final String normalised = format.toLowerCase().trim(); + return matchFormat(format).orElse(DEFAULT_FORMAT); + } + + /** + * Parses an explicit {@code _format} parameter value strictly. A null or blank value maps to the + * default (NDJSON); a non-blank value that matches no supported code or media type is rejected. + * + * @param format the explicit {@code _format} value to parse, or null/blank for the default + * @return the corresponding format + * @throws InvalidRequestException if the value is non-blank and not a supported format + */ + @Nonnull + public static SqlQueryOutputFormat fromStringStrict(@Nullable final String format) { + if (isNullOrBlank(format)) { + return DEFAULT_FORMAT; + } + return matchFormat(format) + .orElseThrow( + () -> + new InvalidRequestException( + ("Unsupported _format value '%s'. Supported formats: ndjson, csv, json," + + " parquet, fhir.") + .formatted(format))); + } + + /** + * Matches a format string against the supported codes and content types. Any media-type + * parameters (e.g. {@code text/csv;charset=utf-8}) are stripped before matching, so a supported + * media type carrying parameters is treated as that format. + */ + @Nonnull + private static Optional matchFormat(@Nonnull final String format) { + final String base = format.split(";", 2)[0].trim().toLowerCase(); return Arrays.stream(values()) - .filter(f -> f.code.equals(normalised) || f.contentType.equals(normalised)) - .findFirst() - .orElse(DEFAULT_FORMAT); + .filter(f -> f.code.equals(base) || f.contentType.equals(base)) + .findFirst(); } /** diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryPipeline.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryPipeline.java new file mode 100644 index 0000000000..097890d814 --- /dev/null +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryPipeline.java @@ -0,0 +1,137 @@ +/* + * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research + * Organisation (CSIRO) ABN 41 687 119 230. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package au.csiro.pathling.operations.sqlquery; + +import au.csiro.pathling.io.source.DataSource; +import au.csiro.pathling.views.FhirView; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.util.Map; +import java.util.function.Consumer; +import org.apache.spark.sql.Dataset; +import org.apache.spark.sql.Row; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.r4.model.BooleanType; +import org.hl7.fhir.r4.model.IntegerType; +import org.hl7.fhir.r4.model.Parameters; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Component; + +/** + * The shared request/execution pipeline for SQL queries. Given a SQLQuery Library, optional runtime + * parameters, and optional request-supplied views, it parses the query, resolves its ViewDefinition + * table sources, statically validates the SQL, and executes it against Spark. + * + *

    Both the synchronous {@code $sqlquery-run} operation (which streams the single result) and the + * asynchronous {@code $sqlquery-export} operation (which writes each result to files) call this + * pipeline, so the parsing, view resolution, validation, and execution semantics are identical + * across the two. Only the terminal step (stream-to-response vs. write-to-files) differs and is + * supplied by the caller as a {@link Consumer} of the result dataset. + * + * @author John Grimes + */ +@Component +public class SqlQueryPipeline { + + @Nonnull private final SqlQueryRequestParser requestParser; + + @Nonnull private final SqlDependencyResolver dependencyResolver; + + @Nonnull private final SqlQueryExecutor executor; + + /** + * Constructs a new SqlQueryPipeline. + * + * @param requestParser parses a SQLQuery or SQLView Library and binds runtime parameters + * @param dependencyResolver resolves the transitive dependency graph, preferring request-supplied + * views over server storage + * @param executor validates and runs the SQL against Spark + */ + @Autowired + public SqlQueryPipeline( + @Nonnull final SqlQueryRequestParser requestParser, + @Nonnull final SqlDependencyResolver dependencyResolver, + @Nonnull final SqlQueryExecutor executor) { + this.requestParser = requestParser; + this.dependencyResolver = dependencyResolver; + this.executor = executor; + } + + /** + * Parses the SQLQuery Library and resolves its view table sources, producing a {@link + * PreparedSqlQuery} ready for validation and execution. Performs all structural FHIR-level + * validation (query parsing, parameter binding and type checking) and view resolution (preferring + * request-supplied views, falling back to server storage), but does not touch Spark. + * + * @param library the SQLQuery Library resource (inline or already resolved from a reference) + * @param format the explicit {@code _format} parameter, if any + * @param acceptHeader the HTTP {@code Accept} header value, used as a fallback for {@code format} + * @param includeHeader whether to include a CSV header row; {@code null} defaults to {@code true} + * @param limit optional row cap + * @param parameters runtime parameter bindings as a {@code Parameters} resource + * @param suppliedViews request-supplied views keyed by the ViewDefinition id they satisfy + * @return the prepared query + */ + @Nonnull + @SuppressWarnings("java:S107") + public PreparedSqlQuery prepare( + @Nonnull final IBaseResource library, + @Nullable final String format, + @Nullable final String acceptHeader, + @Nullable final BooleanType includeHeader, + @Nullable final IntegerType limit, + @Nullable final Parameters parameters, + @Nonnull final Map suppliedViews) { + final SqlQueryRequest request = + requestParser.parse(library, format, acceptHeader, includeHeader, limit, parameters); + final ResolvedDependencyGraph dependencyGraph = + dependencyResolver.resolve(request.getParsedQuery(), suppliedViews); + return new PreparedSqlQuery(request, dependencyGraph); + } + + /** + * Runs the static SQL validation that does not require executing the query, so that malformed or + * disallowed SQL is detected before any Spark work. Validates the top-level SQL and every SQLView + * node's SQL against its own declared labels. Used by the asynchronous export to surface these + * failures synchronously at kick-off. + * + * @param prepared the prepared query + */ + public void validateStatically(@Nonnull final PreparedSqlQuery prepared) { + executor.validateStatically(prepared.getRequest(), prepared.getDependencyGraph()); + } + + /** + * Executes the prepared query against Spark, materialising the resolved dependency graph under + * request-scoped temp views for the duration of the call and invoking {@code consumer} with the + * result dataset before they are dropped. + * + * @param prepared the prepared query + * @param dataSource the data source backing FhirView execution (filtered for the export filters) + * @param requestId the HAPI per-request id used to namespace temp view names + * @param consumer terminal consumer of the result dataset + */ + public void execute( + @Nonnull final PreparedSqlQuery prepared, + @Nonnull final DataSource dataSource, + @Nonnull final String requestId, + @Nonnull final Consumer> consumer) { + executor.execute( + prepared.getRequest(), prepared.getDependencyGraph(), dataSource, requestId, consumer); + } +} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryRequestParser.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryRequestParser.java index 8a566a8d40..a9e5655548 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryRequestParser.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryRequestParser.java @@ -46,6 +46,8 @@ * Validates and normalises the raw HTTP inputs of a {@code $sqlquery-run} invocation into a {@link * SqlQueryRequest}. Has no Spark dependency; performs only structural FHIR-level validation and * parsing. + * + * @author John Grimes */ @Slf4j @Component @@ -59,7 +61,7 @@ public class SqlQueryRequestParser { private static final Set INTEGER_LIKE_TYPES = Set.of("integer", "unsignedInt", "positiveInt"); - @Nonnull private final SqlQueryLibraryParser libraryParser; + @Nonnull private final SqlLibraryParser libraryParser; /** * Constructs a new SqlQueryRequestParser. @@ -67,7 +69,7 @@ public class SqlQueryRequestParser { * @param libraryParser parser for the SQLQuery Library profile */ @Autowired - public SqlQueryRequestParser(@Nonnull final SqlQueryLibraryParser libraryParser) { + public SqlQueryRequestParser(@Nonnull final SqlLibraryParser libraryParser) { this.libraryParser = libraryParser; } @@ -135,8 +137,10 @@ private Library castToLibrary(@Nonnull final IBaseResource resource) { @Nonnull private SqlQueryOutputFormat selectOutputFormat( @Nullable final String format, @Nullable final String acceptHeader) { + // An explicit _format parameter is parsed strictly (an unsupported value is rejected), while + // Accept-header negotiation remains lenient and falls back to NDJSON. if (format != null && !format.isBlank()) { - return SqlQueryOutputFormat.fromString(format); + return SqlQueryOutputFormat.fromStringStrict(format); } return SqlQueryOutputFormat.fromAcceptHeader(acceptHeader); } diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryResultStreamer.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryResultStreamer.java index 9fde319fe8..6c8dec46df 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryResultStreamer.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryResultStreamer.java @@ -17,6 +17,7 @@ package au.csiro.pathling.operations.sqlquery; +import au.csiro.pathling.operations.ParquetSchemaValidator; import au.csiro.pathling.operations.view.ResultStreamingHelper; import au.csiro.pathling.views.ViewDefinitionGson; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; @@ -141,6 +142,10 @@ private void streamParquet( @Nonnull final Dataset result, @Nonnull final HttpServletResponse response) throws IOException { + // Reject unresolved (VOID) columns before any filesystem work, since Spark's Parquet writer + // would otherwise fail with an opaque internal error. + ParquetSchemaValidator.validateSchemaForParquet(result.schema()); + final Path tempDir = Files.createTempDirectory("sqlquery-parquet-", OWNER_ONLY_DIR_ATTRS); final String outputPath = tempDir.resolve("result").toString(); diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunProvider.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunProvider.java index 7f8c09f137..b37c2db6ae 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunProvider.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunProvider.java @@ -39,6 +39,7 @@ *

    This provides a system-level operation at {@code /fhir/$sqlquery-run} that accepts a SQLQuery * Library resource inline or by reference. * + * @author John Grimes * @see SQLQueryRun * @see SqlQueryInstanceRunProvider for type-level and instance-level operations @@ -69,6 +70,7 @@ public SqlQueryRunProvider(@Nonnull final SqlQueryExecutionHelper executionHelpe * @param includeHeader whether to include a header row in CSV output * @param limit the maximum number of rows to return * @param parameters runtime parameter bindings as a Parameters resource + * @param source the unsupported external data source parameter, rejected when supplied * @param requestDetails the servlet request details containing HTTP headers * @param response the HTTP response for streaming output */ @@ -82,9 +84,12 @@ public void run( @Nullable @OperationParam(name = "header") final BooleanType includeHeader, @Nullable @OperationParam(name = "_limit") final IntegerType limit, @Nullable @OperationParam(name = "parameters") final Parameters parameters, + @Nullable @OperationParam(name = "source") final String source, @Nonnull final ServletRequestDetails requestDetails, @Nullable final HttpServletResponse response) { + executionHelper.rejectSourceParameter(source); + final String acceptHeader = requestDetails.getServletRequest().getHeader("Accept"); executionHelper.executeSqlQuery( diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryWatchdog.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryWatchdog.java deleted file mode 100644 index 75cc35ed01..0000000000 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryWatchdog.java +++ /dev/null @@ -1,140 +0,0 @@ -/* - * Copyright © 2018-2026 Commonwealth Scientific and Industrial Research - * Organisation (CSIRO) ABN 41 687 119 230. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package au.csiro.pathling.operations.sqlquery; - -import au.csiro.pathling.config.ServerConfiguration; -import au.csiro.pathling.config.SqlQueryConfiguration; -import jakarta.annotation.Nonnull; -import jakarta.annotation.PreDestroy; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.ScheduledFuture; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicBoolean; -import lombok.extern.slf4j.Slf4j; -import org.apache.spark.sql.SparkSession; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.stereotype.Component; - -/** - * Schedules a wall-clock timeout against each {@code $sqlquery-run} query and cancels the - * associated Spark job group when the timeout fires. Used to defend the synchronous {@code - * $sqlquery-run} surface against queries that consume an unbounded share of compute resources - for - * example, generators chained with array-builder functions that produce few output rows but - * traverse very large intermediate sets, which the row cap cannot short-circuit. - * - *

    Mirrors the cancellation pattern used by {@code AsyncAspect}: {@code setJobGroup(..., - * interruptOnCancel=true)} pairs with {@code cancelJobGroup}, which propagates cancellation to - * in-flight Spark task threads as a thrown exception. - * - * @author John Grimes - */ -@Slf4j -@Component -public class SqlQueryWatchdog { - - @Nonnull private final SparkSession sparkSession; - - @Nonnull private final SqlQueryConfiguration config; - - @Nonnull private final ScheduledExecutorService scheduler; - - /** - * Constructs a new SqlQueryWatchdog. - * - * @param sparkSession the Spark session whose job groups will be cancelled on timeout - * @param serverConfiguration the server configuration, used to resolve the timeout value - */ - @Autowired - public SqlQueryWatchdog( - @Nonnull final SparkSession sparkSession, - @Nonnull final ServerConfiguration serverConfiguration) { - this.sparkSession = sparkSession; - this.config = serverConfiguration.getSqlQuery(); - this.scheduler = - Executors.newSingleThreadScheduledExecutor( - r -> { - final Thread t = new Thread(r, "sqlquery-watchdog"); - t.setDaemon(true); - return t; - }); - } - - /** - * Starts a watchdog for the given Spark job group. The returned {@link Watch} must be completed - * by the caller in a {@code finally} block. - * - * @param jobGroupId the Spark job group id to cancel if the timeout fires - * @return a handle that exposes whether the timeout fired and lets the caller cancel the - * scheduled cancellation when the query completes normally - */ - @Nonnull - public Watch start(@Nonnull final String jobGroupId) { - final long timeoutSeconds = config.getTimeoutSeconds(); - final AtomicBoolean timedOut = new AtomicBoolean(false); - final ScheduledFuture task = - scheduler.schedule( - () -> { - timedOut.set(true); - log.warn( - "$sqlquery-run timeout fired for jobGroupId={} after {} seconds; cancelling.", - jobGroupId, - timeoutSeconds); - try { - sparkSession.sparkContext().cancelJobGroup(jobGroupId); - } catch (final RuntimeException e) { - log.warn("Failed to cancel job group {}: {}", jobGroupId, e.getMessage()); - } - }, - timeoutSeconds, - TimeUnit.SECONDS); - return new Watch(task, timedOut); - } - - /** Shuts down the scheduler when the bean is destroyed. */ - @PreDestroy - public void shutdown() { - scheduler.shutdownNow(); - } - - /** Handle returned by {@link #start(String)}. */ - public static final class Watch { - - @Nonnull private final ScheduledFuture task; - - @Nonnull private final AtomicBoolean timedOut; - - Watch(@Nonnull final ScheduledFuture task, @Nonnull final AtomicBoolean timedOut) { - this.task = task; - this.timedOut = timedOut; - } - - /** - * Cancels the scheduled cancellation. Idempotent. Safe to call after the timeout has already - * fired. - */ - public void complete() { - task.cancel(false); - } - - /** Returns true if the timeout fired before {@link #complete()} was called. */ - public boolean timedOut() { - return timedOut.get(); - } - } -} diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlValidator.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlValidator.java index 99785493aa..2df0a6af6d 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlValidator.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlValidator.java @@ -28,14 +28,20 @@ import org.apache.spark.sql.catalyst.FunctionIdentifier; import org.apache.spark.sql.catalyst.analysis.UnresolvedFunction; import org.apache.spark.sql.catalyst.analysis.UnresolvedRelation; +import org.apache.spark.sql.catalyst.analysis.UnresolvedTableOrView; import org.apache.spark.sql.catalyst.catalog.HiveTableRelation; import org.apache.spark.sql.catalyst.expressions.Expression; import org.apache.spark.sql.catalyst.expressions.ExpressionInfo; import org.apache.spark.sql.catalyst.expressions.SubqueryExpression; +import org.apache.spark.sql.catalyst.expressions.WindowSpecDefinition; import org.apache.spark.sql.catalyst.plans.logical.Command; +import org.apache.spark.sql.catalyst.plans.logical.DescribeRelation; import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan; import org.apache.spark.sql.catalyst.plans.logical.SubqueryAlias; import org.apache.spark.sql.catalyst.plans.logical.UnresolvedWith; +import org.apache.spark.sql.catalyst.plans.logical.WithWindowDefinition; +import org.apache.spark.sql.execution.command.DescribeQueryCommand; +import org.apache.spark.sql.execution.command.DescribeTableCommand; import org.apache.spark.sql.execution.datasources.LogicalRelation; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; @@ -82,6 +88,17 @@ * walks. * * + *

    Two schema-introspection statements are carved out of the blanket {@link Command} rejection: + * {@code DESCRIBE [TABLE]

      + *
    • {@link DescribeRelation} ({@code DESCRIBE [TABLE]
    • {@link DescribeQueryCommand} ({@code DESCRIBE [QUERY] }) is accepted after its + * inner query plan is strictly validated with the same allowed-label set (extended with any + * CTEs the inner query defines). The inner plan is a constructor argument rather than a + * tree child, so it must be walked explicitly. + *
    + * + * @return {@code true} when the node is a recognised, validated DESCRIBE form and the caller + * should not descend into it; {@code false} when the node is not a DESCRIBE form at all + */ + private boolean validateDescribeStrict( + @Nonnull final LogicalPlan plan, @Nonnull final Set allowedLabels) { + if (plan instanceof final DescribeRelation describe) { + if (describe.isExtended()) { + throw new InvalidRequestException( + "SQL contains a disallowed operation: DESCRIBE EXTENDED / FORMATTED"); + } + if (!describe.partitionSpec().isEmpty()) { + throw new InvalidRequestException( + "SQL contains a disallowed operation: DESCRIBE with a partition specification"); + } + if (describe.child() instanceof final UnresolvedTableOrView target) { + validateSinglePartLabel( + CollectionConverters.asJava(target.multipartIdentifier()), allowedLabels); + return true; + } + // An unexpected target shape falls through to the generic Command rejection. + return false; + } + if (plan instanceof final DescribeQueryCommand describeQuery) { + final LogicalPlan innerPlan = describeQuery.plan(); + final Set innerLabels = new HashSet<>(allowedLabels); + collectCteNames(innerPlan, innerLabels); + walkPlanStrict(innerPlan, innerLabels); + return true; + } + return false; + } + /** Recursively validates an analyzed plan, tracking whether we are inside a trusted alias. */ private void walkPlanAnalyzed( @Nonnull final LogicalPlan plan, @@ -428,6 +553,16 @@ private void walkPlanAnalyzed( for (final Expression expr : expressions) { walkExpressionAnalyzed(expr, registeredViewNames); } + // Defence in depth: mirror the strict-walk carve-out for any WithWindowDefinition that + // survives analysis (for example a pipe-SQL WINDOW clause), whose windowDefinitions map + // is not reached by plan.expressions(). The authoritative gate is the strict walk in + // validate; ordinary SQL substitutes named windows into Window nodes before this runs. + if (plan instanceof final WithWindowDefinition withWindow) { + for (final WindowSpecDefinition spec : + CollectionConverters.asJava(withWindow.windowDefinitions()).values()) { + walkExpressionAnalyzed(spec, registeredViewNames); + } + } final boolean childTrust = inTrustedAlias || isTrustedAlias(plan, registeredViewNames); final List children = CollectionConverters.asJava(plan.children()); for (final LogicalPlan child : children) { @@ -520,6 +655,12 @@ private void validatePlanNodeAnalyzed( @Nonnull final LogicalPlan plan, @Nonnull final Set registeredViewNames, final boolean inTrustedAlias) { + if (isAllowedDescribeAnalyzed(plan, registeredViewNames)) { + // A DESCRIBE command already fully vetted at parse time. Spark rewrites the parsed + // DescribeRelation into a DescribeTableCommand during analysis, so it is recognised here by + // its analysed form. This is defence in depth only; the parse-time gate is authoritative. + return; + } if (plan instanceof Command) { throw new InvalidRequestException( "SQL contains a disallowed operation: " + plan.getClass().getSimpleName()); @@ -534,6 +675,44 @@ private void validatePlanNodeAnalyzed( } } + /** + * Recognises the analysed form of the two allowed {@code DESCRIBE} statements. Spark's {@code + * ResolveSessionCatalog} rewrites a parsed {@link DescribeRelation} over a (temp) view into a + * {@link DescribeTableCommand}, so the table form is matched by that class here rather than by + * {@link DescribeRelation}. + * + *
      + *
    • {@link DescribeTableCommand} is accepted only when it is not extended, carries no + * partition specification, and its table name matches one of the request-scoped temp views + * (case-insensitively, matching {@link #isTrustedAlias} for the same catalog-normalisation + * reason). Any other target would indicate the parse-time label check was bypassed. + *
    • {@link DescribeQueryCommand} is accepted unconditionally: its inner query plan was + * strictly validated at parse time and is only ever analysed - never executed - by the + * command, so no analysed-plan re-check is required. + *
    + * + * @return {@code true} when the node is an allowed analysed DESCRIBE form; {@code false} + * otherwise, so a disallowed describe falls through to the blanket {@link Command} rejection + */ + private static boolean isAllowedDescribeAnalyzed( + @Nonnull final LogicalPlan plan, @Nonnull final Set registeredViewNames) { + if (plan instanceof DescribeQueryCommand) { + return true; + } + if (plan instanceof final DescribeTableCommand describe) { + if (describe.isExtended() || !describe.partitionSpec().isEmpty()) { + return false; + } + final String tableName = describe.table().table(); + for (final String registered : registeredViewNames) { + if (registered.equalsIgnoreCase(tableName)) { + return true; + } + } + } + return false; + } + /** * Enforces that an unresolved relation is a single-part identifier matching {@link * #LABEL_PATTERN} and present in the allowed-label set. Rejects two-part datasource short-name @@ -541,7 +720,17 @@ private void validatePlanNodeAnalyzed( */ private static void validateRelationReference( @Nonnull final UnresolvedRelation relation, @Nonnull final Set allowedLabels) { - final List parts = CollectionConverters.asJava(relation.multipartIdentifier()); + validateSinglePartLabel( + CollectionConverters.asJava(relation.multipartIdentifier()), allowedLabels); + } + + /** + * Enforces that a relation identifier is a single-part identifier matching {@link #LABEL_PATTERN} + * and present in the allowed-label set. Shared by ordinary relation references and the {@code + * DESCRIBE [TABLE]