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/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/server/pom.xml b/server/pom.xml index 9a2c24f1f6..3d970df468 100644 --- a/server/pom.xml +++ b/server/pom.xml @@ -36,7 +36,7 @@ 21 UTF-8 1 - 9.7.1 + 9.8.0 8.10.0 3.5.14 4.0.2 @@ -56,6 +56,17 @@ 2.40.3 1.0.4 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 @@ -585,7 +596,9 @@ spring-boot-maven-plugin ${pathling.springBootVersion} - --add-opens=java.base/sun.nio.ch=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED + + ${pathling.runtime.jvmModuleOpts} @@ -601,22 +614,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} @@ -639,22 +637,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} 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..94ebe1358e 100644 --- a/server/src/main/java/au/csiro/pathling/config/SqlQueryConfiguration.java +++ b/server/src/main/java/au/csiro/pathling/config/SqlQueryConfiguration.java @@ -49,4 +49,14 @@ public class SqlQueryConfiguration { */ @Min(1) private long timeoutSeconds = 60L; + + /** + * 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 int maxDependencyDepth = 10; } 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 index a89abf1e49..12b2963613 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/PreparedSqlQuery.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/PreparedSqlQuery.java @@ -17,16 +17,16 @@ package au.csiro.pathling.operations.sqlquery; -import au.csiro.pathling.views.FhirView; import jakarta.annotation.Nonnull; -import java.util.Map; import lombok.Value; /** - * A SQL query that has been parsed and had its ViewDefinition table sources resolved, ready for - * static validation and execution by {@link SqlQueryPipeline}. Produced by {@link + * 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 { @@ -34,6 +34,9 @@ public class PreparedSqlQuery { /** The validated, normalised request: parsed query, output format, header flag, bindings. */ @Nonnull SqlQueryRequest request; - /** The resolved view table sources the SQL references, keyed by table label. */ - @Nonnull Map resolvedViews; + /** + * 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/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/SqlQueryExecutor.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExecutor.java index 54e430efc8..49e3a44acc 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 @@ -20,13 +20,12 @@ 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 +34,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 @@ -79,47 +86,66 @@ public SqlQueryExecutor( } /** - * 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. * * @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 Map registeredByKey = new LinkedHashMap<>(); final SqlQueryWatchdog.Watch watch = watchdog.start(jobGroupId); 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)); @@ -135,8 +161,62 @@ public void execute( } finally { watch.complete(); sparkSession.sparkContext().clearJobGroup(); - viewRegistrationService.dropViews(registeredViews.values()); + viewRegistrationService.dropViews(registeredByKey.values()); + } + } + + /** + * 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. + */ + 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()); + } + 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 labelToViewName; } /** 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 index 82e7dd57b7..07ab5360f6 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportRequestParser.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportRequestParser.java @@ -18,6 +18,7 @@ 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; @@ -225,10 +226,11 @@ private IBaseResource selectLibrary( } /** - * Resolves the {@code view} parts into a map keyed by the ViewDefinition id they satisfy, parsing + * 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). + * 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) { @@ -253,6 +255,20 @@ private Map resolveSuppliedViews(@Nonnull final Parameters par 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 @@ -264,10 +280,7 @@ private Map resolveSuppliedViews(@Nonnull final Parameters par validateViewSemantically(view); - final String matchKey = resolvedResource.getIdElement().getIdPart(); - if (matchKey != null && !matchKey.isBlank()) { - resolved.put(matchKey, view); - } + resolved.put(url, view); } return resolved; } 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 index 3c8c7086b8..fb6accd423 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportSupport.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportSupport.java @@ -233,7 +233,8 @@ public String computeCacheKeyComponent(@Nonnull final SqlQueryExportRequest requ + ":" + q.preparedQuery().getRequest().getParsedQuery().getSql() + ":" - + gson.toJson(q.preparedQuery().getResolvedViews()) + + gson.toJson( + describeDependencyGraph(q.preparedQuery().getDependencyGraph())) + ":" + gson.toJson(q.preparedQuery().getRequest().getParameterBindings())) .collect(Collectors.joining(",")); @@ -256,6 +257,33 @@ public String computeCacheKeyComponent(@Nonnull final SqlQueryExportRequest requ 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) { 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 index d994f5bbe2..097890d814 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryPipeline.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/SqlQueryPipeline.java @@ -22,9 +22,7 @@ import jakarta.annotation.Nonnull; import jakarta.annotation.Nullable; import java.util.Map; -import java.util.Set; import java.util.function.Consumer; -import java.util.stream.Collectors; import org.apache.spark.sql.Dataset; import org.apache.spark.sql.Row; import org.hl7.fhir.instance.model.api.IBaseResource; @@ -52,32 +50,26 @@ public class SqlQueryPipeline { @Nonnull private final SqlQueryRequestParser requestParser; - @Nonnull private final ViewResolver viewResolver; + @Nonnull private final SqlDependencyResolver dependencyResolver; @Nonnull private final SqlQueryExecutor executor; - @Nonnull private final SqlValidator sqlValidator; - /** * Constructs a new SqlQueryPipeline. * - * @param requestParser parses a SQLQuery Library and binds runtime parameters - * @param viewResolver resolves the view references to parsed FhirViews, preferring - * request-supplied views over server storage + * @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 - * @param sqlValidator the static SQL validator, used for kick-off-time validation that does not - * require executing the query */ @Autowired public SqlQueryPipeline( @Nonnull final SqlQueryRequestParser requestParser, - @Nonnull final ViewResolver viewResolver, - @Nonnull final SqlQueryExecutor executor, - @Nonnull final SqlValidator sqlValidator) { + @Nonnull final SqlDependencyResolver dependencyResolver, + @Nonnull final SqlQueryExecutor executor) { this.requestParser = requestParser; - this.viewResolver = viewResolver; + this.dependencyResolver = dependencyResolver; this.executor = executor; - this.sqlValidator = sqlValidator; } /** @@ -107,30 +99,27 @@ public PreparedSqlQuery prepare( @Nonnull final Map suppliedViews) { final SqlQueryRequest request = requestParser.parse(library, format, acceptHeader, includeHeader, limit, parameters); - final Map resolvedViews = - viewResolver.resolve(request.getParsedQuery().getViewReferences(), suppliedViews); - return new PreparedSqlQuery(request, resolvedViews); + 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. Used by the asynchronous export to surface - * these failures synchronously at kick-off. + * 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) { - final Set declaredLabels = - prepared.getRequest().getParsedQuery().getViewReferences().stream() - .map(ViewArtifactReference::getLabel) - .collect(Collectors.toUnmodifiableSet()); - sqlValidator.validate(prepared.getRequest().getParsedQuery().getSql(), declaredLabels); + executor.validateStatically(prepared.getRequest(), prepared.getDependencyGraph()); } /** - * Executes the prepared query against Spark, registering the resolved views under request-scoped - * temp views for the duration of the call and invoking {@code consumer} with the result dataset - * before they are dropped. + * 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) @@ -143,6 +132,6 @@ public void execute( @Nonnull final String requestId, @Nonnull final Consumer> consumer) { executor.execute( - prepared.getRequest(), prepared.getResolvedViews(), dataSource, requestId, consumer); + 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 202f2856bc..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 @@ -61,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. @@ -69,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; } 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 a6b008fceb..576c0b6873 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 @@ -217,6 +217,10 @@ public class SqlValidator { "org.apache.spark.sql.catalyst.expressions.StringInstr", "org.apache.spark.sql.catalyst.expressions.StringLocate", "org.apache.spark.sql.catalyst.expressions.FindInSet", + // String and array concatenation. The || operator parses directly to a Concat + // expression rather than an UnresolvedFunction, so it must be allow-listed in its own + // right to stay consistent with the concat() function, which is already permitted. + "org.apache.spark.sql.catalyst.expressions.Concat", // Logical operators. "org.apache.spark.sql.catalyst.expressions.And", "org.apache.spark.sql.catalyst.expressions.Or", @@ -317,9 +321,9 @@ public SqlValidator(@Nonnull final SparkSession sparkSession) { } /** - * Pattern that legitimate relation identifiers must match. Mirrors the {@code - * SqlQueryLibraryParser} label pattern so that anything Spark's parser produces as an - * UnresolvedRelation but which could not have been a declared label is rejected outright. + * Pattern that legitimate relation identifiers must match. Mirrors the {@code SqlLibraryParser} + * label pattern so that anything Spark's parser produces as an UnresolvedRelation but which could + * not have been a declared label is rejected outright. */ private static final Pattern LABEL_PATTERN = Pattern.compile("^[A-Za-z]\\w*$"); @@ -417,6 +421,18 @@ private void walkPlanStrict( for (final Expression expr : expressions) { walkExpressionStrict(expr, allowedLabels); } + // A WITH node exposes its CTE definition bodies as innerChildren, not children, so the generic + // child walk below never reaches them. Walk them explicitly so that relation references inside + // a CTE body are validated against the declared-label set, just like top-level relations. The + // CTE names themselves are already in allowedLabels (collectCteNames runs first), so a CTE body + // may legitimately reference a sibling or declared label but not an undeclared external table. + if (plan instanceof final UnresolvedWith with) { + final List> ctes = + CollectionConverters.asJava(with.cteRelations()); + for (final Tuple2 cte : ctes) { + walkPlanStrict(cte._2(), allowedLabels); + } + } final List children = CollectionConverters.asJava(plan.children()); for (final LogicalPlan child : children) { walkPlanStrict(child, allowedLabels); diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/ViewArtifactReference.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ViewArtifactReference.java index 4422017244..ea82da417b 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/ViewArtifactReference.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ViewArtifactReference.java @@ -21,16 +21,23 @@ import lombok.Value; /** - * Represents a reference to a ViewDefinition dependency within a SQLQuery Library resource. Each - * reference has a label (the table alias used in the SQL) and a canonical URL pointing to the - * ViewDefinition. + * Represents a dependency reference within a SQLQuery or SQLView Library resource (a {@code + * relatedArtifact} of type {@code depends-on}). Each reference has a label (the table name used in + * the SQL) and the canonical URL of the ViewDefinition or SQLView that backs that table. The + * canonical URL is matched against the referenced resource's {@code url} element; the logical id + * plays no part in resolution. + * + * @author John Grimes */ @Value public class ViewArtifactReference { - /** The table alias used in the SQL query to reference this ViewDefinition. */ + /** The table name used in the SQL query to reference this dependency. */ @Nonnull String label; - /** The canonical URL or relative reference to the ViewDefinition resource. */ + /** + * The absolute canonical URL of the referenced ViewDefinition or SQLView, optionally suffixed + * with {@code |version}. + */ @Nonnull String canonicalUrl; } diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/ViewRegistrationService.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ViewRegistrationService.java index b6d1761de9..ffdd761b2c 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/ViewRegistrationService.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ViewRegistrationService.java @@ -38,8 +38,12 @@ /** * Manages the lifecycle of Spark temporary views for SQL query execution. Each execution scopes its - * views to the HAPI per-request id so that concurrent {@code $sqlquery-run} requests using the same - * label cannot clobber one another in Spark's session-global temporary view catalog. + * views to the HAPI per-request id and keys them by the resolved dependency's canonical identity, + * so concurrent {@code $sqlquery-run} requests cannot clobber one another in Spark's session-global + * temporary view catalog, a shared node materialises once, and the same table label used in + * different nodes cannot collide. + * + * @author John Grimes */ @Slf4j @Component @@ -73,63 +77,84 @@ public ViewRegistrationService( } /** - * Registers resolved ViewDefinitions as Spark temporary views, scoped by the request id so that - * concurrent executions cannot clobber one another in the session-global catalog. + * Registers an already-built dataset under a request-scoped temp view name derived from the given + * identifier (a dependency's canonical key, or a bare label in the single-level tests) and + * returns that name. * - * @param resolvedViews a map from label (table alias) to the parsed FhirView - * @param dataSource the data source to use for view execution - * @param requestId the per-request id used to namespace registered view names - * @return a map from original label to the actual temporary view name + * @param identifier the canonical key (or label) the temp view materialises + * @param dataset the dataset to register + * @param requestId the per-request id used to namespace the registered view name + * @return the registered temp view name */ @Nonnull - public Map registerViews( - @Nonnull final Map resolvedViews, - @Nonnull final DataSource dataSource, + String registerDataset( + @Nonnull final String identifier, + @Nonnull final Dataset dataset, @Nonnull final String requestId) { + final String tempViewName = resolveTempViewName(requestId, identifier); + dataset.createOrReplaceTempView(tempViewName); + return tempViewName; + } - final Map labelToViewName = new LinkedHashMap<>(); - - for (final Map.Entry entry : resolvedViews.entrySet()) { - final String label = entry.getKey(); - final FhirView view = entry.getValue(); - - final FhirViewExecutor executor = - new FhirViewExecutor(fhirContext, dataSource, queryConfiguration); - final Dataset result; - try { - result = executor.buildQuery(view); - } catch (final Exception e) { - // Drop any views that were already registered before failing. - dropViews(labelToViewName.values()); - throw new InvalidRequestException( - "Failed to execute ViewDefinition for label '" + label + "': " + e.getMessage()); - } - - final String tempViewName = registerDataset(label, result, requestId); - labelToViewName.put(label, tempViewName); - log.info( - "Registered temporary view '{}' for label '{}' (resource type '{}')", - tempViewName, - label, - view.getResource()); + /** + * Builds the dataset for a resolved {@code ViewDefinition} leaf by executing its parsed view + * against the data source. The result is not registered; the caller registers it once any + * required validation has passed. + * + * @param view the parsed view to execute + * @param dataSource the data source backing view execution + * @return the view's result dataset + * @throws InvalidRequestException if the view cannot be executed + */ + @Nonnull + public Dataset buildViewDefinition( + @Nonnull final FhirView view, @Nonnull final DataSource dataSource) { + final FhirViewExecutor executor = + new FhirViewExecutor(fhirContext, dataSource, queryConfiguration); + try { + return executor.buildQuery(view); + } catch (final Exception e) { + throw new InvalidRequestException( + "Failed to execute ViewDefinition for resource type '" + + view.getResource() + + "': " + + e.getMessage()); } - - return labelToViewName; } /** - * Registers an already-built dataset under a request-scoped temp view name and returns that name. - * Visible for tests that need to exercise the temp-view namespacing without going through - * FhirViewExecutor. + * Builds the dataset for a resolved {@code SQLView} node by rewriting its SQL so each of its + * table labels points at the already-registered temp view of the child it resolves to, then + * running that SQL. The result is not registered; the caller validates the analysed plan and then + * registers it. + * + * @param node the resolved SQLView node + * @param registeredByKey the temp view names of already-materialised nodes, keyed by canonical + * key + * @return the SQLView's result dataset */ @Nonnull - String registerDataset( - @Nonnull final String label, - @Nonnull final Dataset dataset, - @Nonnull final String requestId) { - final String tempViewName = resolveTempViewName(requestId, label); - dataset.createOrReplaceTempView(tempViewName); - return tempViewName; + public Dataset buildSqlView( + @Nonnull final ResolvedSqlView node, @Nonnull final Map registeredByKey) { + final Map labelToViewName = new LinkedHashMap<>(); + node.getChildKeysByLabel() + .forEach( + (label, childKey) -> { + final String viewName = registeredByKey.get(childKey); + if (viewName == null) { + // Topological ordering guarantees children are materialised first; a miss here is + // an internal invariant violation, not a client error. + throw new IllegalStateException( + "Child dependency '" + + childKey + + "' for label '" + + label + + "' was not materialised before the SQLView that references it"); + } + labelToViewName.put(label, viewName); + }); + final String rewrittenSql = rewriteSql(node.getSql(), labelToViewName); + return sparkSession.sql(rewrittenSql); } /** @@ -324,15 +349,22 @@ private static int copyOrRewriteIdentifier( } /** - * Constructs the request-scoped temp view name for a given label. + * Constructs the request-scoped temp view name for a node identifier. The identifier is either a + * dependency's canonical key (the production case, so a shared node materialises once and labels + * cannot collide across nodes) or, for the existing single-level tests, a bare table label. * - *

    The request id is sanitised to keep the resulting identifier valid for use as a Spark temp - * view name (HAPI default is 16 alphanumerics, but {@code X-Request-ID} can carry arbitrary - * characters). + *

    Both the request id and the identifier are sanitised so that the resulting Spark temp view + * name is a legal identifier (HAPI request ids are alphanumeric, but {@code X-Request-ID} and + * canonical keys such as {@code ViewDefinition/patient-view} can carry slashes and dashes). + * + * @param requestId the per-request id used to namespace registered view names + * @param identifier the canonical key (or label) the temp view materialises + * @return the temp view name */ @Nonnull - static String resolveTempViewName(@Nonnull final String requestId, @Nonnull final String label) { - return VIEW_NAME_PREFIX + sanitiseRequestId(requestId) + "_" + label; + static String resolveTempViewName( + @Nonnull final String requestId, @Nonnull final String identifier) { + return VIEW_NAME_PREFIX + sanitiseRequestId(requestId) + "_" + sanitiseIdentifier(identifier); } /** @@ -349,4 +381,20 @@ private static String sanitiseRequestId(@Nonnull final String requestId) { } return "r" + Integer.toUnsignedString(requestId.hashCode(), 16); } + + /** + * Renders a node identifier as a Spark-safe temp view name segment. A bare word identifier (a + * validated table label) is used verbatim, preserving the single-level naming. Any identifier + * containing characters illegal in a Spark identifier (a canonical key such as {@code + * ViewDefinition/patient-view}) has those characters replaced with underscores and a hash of the + * original appended, so that two distinct keys never collapse to the same name. + */ + @Nonnull + private static String sanitiseIdentifier(@Nonnull final String identifier) { + if (!UNSAFE_REQUEST_ID_CHARS.matcher(identifier).find()) { + return identifier; + } + final String cleaned = UNSAFE_REQUEST_ID_CHARS.matcher(identifier).replaceAll("_"); + return cleaned + "_" + Integer.toUnsignedString(identifier.hashCode(), 16); + } } diff --git a/server/src/main/java/au/csiro/pathling/operations/sqlquery/ViewResolver.java b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ViewResolver.java index b1355cb62d..47c6bc0077 100644 --- a/server/src/main/java/au/csiro/pathling/operations/sqlquery/ViewResolver.java +++ b/server/src/main/java/au/csiro/pathling/operations/sqlquery/ViewResolver.java @@ -18,7 +18,9 @@ package au.csiro.pathling.operations.sqlquery; import au.csiro.pathling.config.ServerConfiguration; -import au.csiro.pathling.read.ReadExecutor; +import au.csiro.pathling.encoders.FhirEncoders; +import au.csiro.pathling.encoders.ViewDefinitionResource; +import au.csiro.pathling.io.source.DataSource; import au.csiro.pathling.security.PathlingAuthority; import au.csiro.pathling.security.ResourceAccess.AccessType; import au.csiro.pathling.security.SecurityAspect; @@ -29,17 +31,29 @@ import com.google.gson.Gson; import com.google.gson.JsonSyntaxException; import jakarta.annotation.Nonnull; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Optional; +import org.apache.spark.sql.Dataset; +import org.apache.spark.sql.Row; +import org.apache.spark.sql.catalyst.encoders.ExpressionEncoder; +import org.apache.spark.sql.functions; import org.hl7.fhir.instance.model.api.IBaseResource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; /** - * Resolves the {@link ViewArtifactReference}s declared by a SQLQuery Library into parsed {@link - * FhirView}s, performing the per-resource-type authorisation check along the way. Has no Spark - * dependency; the FhirView is the parsed view configuration, not yet a {@code Dataset}. + * Resolves a {@link ViewArtifactReference} that targets a {@code ViewDefinition} into a {@link + * ResolvedViewDefinition} leaf node, matching the reference's canonical URL against the stored + * {@code ViewDefinition.url} (the logical id plays no part). A request-supplied view is preferred + * over storage, matched by its URL. Has no Spark execution dependency; the {@link FhirView} is the + * parsed view configuration, not yet a {@code Dataset}. + * + *

    Resolution is split into the supplied-view step and the stored-lookup step so the caller can + * detect an ambiguous reference (a URL matching both a {@code ViewDefinition} and a {@code SQLView + * Library}) before committing to either. + * + * @author John Grimes */ // Bean name is set explicitly to avoid colliding with Spring MVC's DispatcherServlet, which // auto-discovers any bean named "viewResolver" and casts it to @@ -47,7 +61,13 @@ @Component("sqlQueryViewResolver") public class ViewResolver { - @Nonnull private final ReadExecutor readExecutor; + private static final String VIEW_DEFINITION = "ViewDefinition"; + + private static final String ACTIVE_STATUS = "active"; + + @Nonnull private final DataSource dataSource; + + @Nonnull private final FhirEncoders fhirEncoders; @Nonnull private final ServerConfiguration serverConfiguration; @@ -58,101 +78,125 @@ public class ViewResolver { /** * Constructs a new ViewResolver. * - * @param readExecutor read executor for stored ViewDefinition resources + * @param dataSource the data source used to match stored ViewDefinitions by url + * @param fhirEncoders FHIR encoders used to decode the matched ViewDefinition rows * @param serverConfiguration server configuration (used for the auth toggle) * @param fhirContext FHIR context used to serialise the resolved ViewDefinition for parsing */ @Autowired public ViewResolver( - @Nonnull final ReadExecutor readExecutor, + @Nonnull final DataSource dataSource, + @Nonnull final FhirEncoders fhirEncoders, @Nonnull final ServerConfiguration serverConfiguration, @Nonnull final FhirContext fhirContext) { - this.readExecutor = readExecutor; + this.dataSource = dataSource; + this.fhirEncoders = fhirEncoders; this.serverConfiguration = serverConfiguration; this.fhirContext = fhirContext; this.gson = ViewDefinitionGson.create(); } /** - * Resolves each view reference to a parsed {@link FhirView}, reading every view from server - * storage. Equivalent to {@link #resolve(List, Map)} with no request-supplied views. + * Resolves a reference against a request-supplied view, preferring it over storage. A supplied + * view satisfies a reference when its URL matches the reference's url. It carries its own + * authorisation as the request payload, so no metadata or projected-resource READ check applies. * - * @param references the references declared by the SQLQuery Library, keyed by table label - * @return a map from label to resolved FhirView - * @throws InvalidRequestException if a reference cannot be resolved or parsed + * @param reference the dependency reference to resolve + * @param suppliedViews request-supplied views keyed by the canonical URL they satisfy + * @return the resolved leaf node, or empty if no supplied view matches the reference's url */ @Nonnull - public Map resolve(@Nonnull final List references) { - return resolve(references, Map.of()); + public Optional resolveSuppliedView( + @Nonnull final ViewArtifactReference reference, + @Nonnull final Map suppliedViews) { + final CanonicalReference canonical = CanonicalReference.parse(reference.getCanonicalUrl()); + final FhirView supplied = suppliedViews.get(canonical.getUrl()); + if (supplied == null) { + return Optional.empty(); + } + // A supplied view carries no version; its identity is its URL. + return Optional.of(new ResolvedViewDefinition(canonical.getUrl(), supplied)); } /** - * Resolves each view reference to a parsed {@link FhirView}, preserving label order. A - * request-supplied view is used in preference to server storage when it matches the reference (by - * the ViewDefinition id extracted from the reference); otherwise the view is read from server - * storage, exactly as {@code $sqlquery-run} does. Request-supplied views are assumed already - * parsed and, for stored references, authorisation-checked by the caller. + * Resolves a reference against a stored {@code ViewDefinition}, matching the reference's url (and + * version, when supplied) against {@code ViewDefinition.url}. Returns empty when no stored + * ViewDefinition matches, so the caller can fall back to a {@code SQLView}. + * + *

    When a ViewDefinition matches, the {@code ViewDefinition} metadata READ check and the + * per-projected-resource-type READ check are enforced (when authorisation is enabled). * - * @param references the references declared by the SQLQuery Library, keyed by table label - * @param suppliedViews request-supplied views keyed by the ViewDefinition id (or canonical url's - * final segment) they satisfy; may be empty - * @return a map from label to resolved FhirView - * @throws InvalidRequestException if a reference cannot be resolved or parsed + * @param reference the dependency reference to resolve + * @return the resolved leaf node, or empty if no stored ViewDefinition matches + * @throws InvalidRequestException if a matching ViewDefinition cannot be parsed */ @Nonnull - public Map resolve( - @Nonnull final List references, - @Nonnull final Map suppliedViews) { - final Map resolved = new LinkedHashMap<>(); - - for (final ViewArtifactReference ref : references) { - final String viewDefinitionId = extractViewDefinitionId(ref.getCanonicalUrl()); + public Optional resolveStoredViewDefinition( + @Nonnull final ViewArtifactReference reference) { + final CanonicalReference canonical = CanonicalReference.parse(reference.getCanonicalUrl()); + final List matches = matchByUrl(canonical); + if (matches.isEmpty()) { + return Optional.empty(); + } - // Prefer a request-supplied view that satisfies this reference, falling back to server - // storage when none is supplied. - final FhirView suppliedView = suppliedViews.get(viewDefinitionId); - if (suppliedView != null) { - resolved.put(ref.getLabel(), suppliedView); - continue; - } + final ViewDefinitionResource chosen = + (ViewDefinitionResource) + canonical.select(matches, ViewResolver::isActive, ViewResolver::versionOf); - final IBaseResource viewResource; - try { - viewResource = readExecutor.read("ViewDefinition", viewDefinitionId); - } catch (final Exception e) { - throw new InvalidRequestException( - "Failed to resolve ViewDefinition for label '" - + ref.getLabel() - + "' with reference '" - + ref.getCanonicalUrl() - + "': " - + e.getMessage()); - } + // The ViewDefinition was read from storage: enforce the metadata READ check, then parse it and + // enforce the per-projected-resource READ check. + checkMetadataReadAuthority(); + final FhirView view = parseViewDefinition(chosen); + checkProjectedResourceReadAuthority(view); - final FhirView view = parseViewDefinition(viewResource); + final String canonicalKey = CanonicalReference.key(chosen.getUrl(), chosen.getVersion()); + return Optional.of(new ResolvedViewDefinition(canonicalKey, view)); + } - if (serverConfiguration.getAuth().isEnabled()) { - SecurityAspect.checkHasAuthority( - PathlingAuthority.resourceAccess(AccessType.READ, view.getResource())); + /** + * Matches stored ViewDefinitions by the reference's url (and version, when supplied), returning + * the decoded resources. When the server holds no ViewDefinition data at all, the reference + * simply does not match, so an empty list is returned rather than surfacing the data source's + * missing-type error. + */ + @Nonnull + private List matchByUrl(@Nonnull final CanonicalReference canonical) { + final Dataset viewDefinitions; + try { + viewDefinitions = dataSource.read(VIEW_DEFINITION); + } catch (final IllegalArgumentException e) { + if (isMissingResourceType(e)) { + return List.of(); } - - resolved.put(ref.getLabel(), view); + throw e; } - - return resolved; + Dataset filtered = + viewDefinitions.filter(viewDefinitions.col("url").equalTo(canonical.getUrl())); + if (canonical.hasVersion()) { + filtered = filtered.filter(functions.col("version").equalTo(canonical.getVersion())); + } + final ExpressionEncoder encoder = fhirEncoders.of(VIEW_DEFINITION); + return filtered.as(encoder).collectAsList(); } /** - * Extracts a ViewDefinition id from a canonical URL or relative reference. Supports relative - * references like {@code ViewDefinition/my-id} and bare ids. + * Indicates whether an {@link IllegalArgumentException} signals that the data source holds no + * data for the requested resource type (as opposed to a genuine error). */ - @Nonnull - private String extractViewDefinitionId(@Nonnull final String canonicalUrl) { - if (canonicalUrl.contains("/")) { - final String[] parts = canonicalUrl.split("/"); - return parts[parts.length - 1]; - } - return canonicalUrl; + private static boolean isMissingResourceType(@Nonnull final IllegalArgumentException e) { + return e.getMessage() != null && e.getMessage().contains("No data found for resource type"); + } + + /** Returns whether a decoded ViewDefinition carries {@code active} status. */ + private static boolean isActive(@Nonnull final IBaseResource resource) { + final ViewDefinitionResource view = (ViewDefinitionResource) resource; + return view.getStatusElement() != null + && ACTIVE_STATUS.equals(view.getStatusElement().getValueAsString()); + } + + /** Returns the version of a decoded ViewDefinition, or null when it has none. */ + private static String versionOf(@Nonnull final IBaseResource resource) { + return ((ViewDefinitionResource) resource).getVersion(); } /** Parses a ViewDefinition resource into a FhirView via JSON round-tripping. */ @@ -165,4 +209,27 @@ private FhirView parseViewDefinition(@Nonnull final IBaseResource viewResource) throw new InvalidRequestException("Invalid ViewDefinition: " + e.getMessage()); } } + + /** + * Enforces the metadata READ check for a ViewDefinition resolved from storage, when authorisation + * is enabled. Reading a {@code ViewDefinition} out of the server requires READ authority on the + * {@code ViewDefinition} type itself, independent of the data the view projects. + */ + private void checkMetadataReadAuthority() { + if (serverConfiguration.getAuth().isEnabled()) { + SecurityAspect.checkHasAuthority( + PathlingAuthority.resourceAccess(AccessType.READ, VIEW_DEFINITION)); + } + } + + /** + * Enforces the per-projected-resource-type READ check for a parsed view, when authorisation is + * enabled. + */ + private void checkProjectedResourceReadAuthority(@Nonnull final FhirView view) { + if (serverConfiguration.getAuth().isEnabled()) { + SecurityAspect.checkHasAuthority( + PathlingAuthority.resourceAccess(AccessType.READ, view.getResource())); + } + } } diff --git a/server/src/main/java/au/csiro/pathling/operations/view/ViewExecutionHelper.java b/server/src/main/java/au/csiro/pathling/operations/view/ViewExecutionHelper.java index 1c798858c8..fac47030e0 100644 --- a/server/src/main/java/au/csiro/pathling/operations/view/ViewExecutionHelper.java +++ b/server/src/main/java/au/csiro/pathling/operations/view/ViewExecutionHelper.java @@ -185,7 +185,10 @@ public IBaseResource resolveViewInput( } /** - * Reads a stored ViewDefinition by its logical id, mapping a missing resource to a 404. + * Reads a stored ViewDefinition by its logical id, mapping a missing resource to a 404. As the + * ViewDefinition is read from server storage, the metadata READ check on {@code ViewDefinition} + * is enforced when authorisation is enabled, layered on top of the per-projected-resource check + * applied later when the view is executed. * * @param id the logical id of the stored ViewDefinition * @return the stored ViewDefinition resource @@ -193,6 +196,10 @@ public IBaseResource resolveViewInput( */ @Nonnull public IBaseResource readStoredViewDefinition(@Nonnull final String id) { + if (serverConfiguration.getAuth().isEnabled()) { + SecurityAspect.checkHasAuthority( + PathlingAuthority.resourceAccess(AccessType.READ, "ViewDefinition")); + } try { return readExecutor.read("ViewDefinition", id); } catch (final ResourceNotFoundError e) { diff --git a/server/src/main/jib/usr/bin/entrypoint.sh b/server/src/main/jib/usr/bin/entrypoint.sh index ee4ed77032..074376b72e 100755 --- a/server/src/main/jib/usr/bin/entrypoint.sh +++ b/server/src/main/jib/usr/bin/entrypoint.sh @@ -18,7 +18,9 @@ set -e # The Pathling JVM module options required on Java 21. Both roles must run with # these, so they are defined once here and referenced from each branch. They # were previously supplied by the Jib build's jvmFlags, which Jib ignores once -# an explicit entrypoint is configured. +# an explicit entrypoint is configured. This list must be kept in sync with the +# pathling.runtime.jvmModuleOpts property in the server pom.xml, which a shell +# script cannot reference at build or run time. PATHLING_JVM_OPTS=( --add-exports=java.base/sun.nio.ch=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED diff --git a/server/src/main/resources/examples/sqlquery-run-examples.md b/server/src/main/resources/examples/sqlquery-run-examples.md index ed6f51a1d5..494586c223 100644 --- a/server/src/main/resources/examples/sqlquery-run-examples.md +++ b/server/src/main/resources/examples/sqlquery-run-examples.md @@ -63,6 +63,7 @@ curl -s -X POST "http://localhost:8080/fhir/ViewDefinition" \ -H "Content-Type: application/fhir+json" \ -d '{ "resourceType": "ViewDefinition", + "url": "https://example.org/ViewDefinition/patients", "name": "patients", "resource": "Patient", "select": [ @@ -91,6 +92,7 @@ curl -s -X POST "http://localhost:8080/fhir/ViewDefinition" \ -H "Content-Type: application/fhir+json" \ -d '{ "resourceType": "ViewDefinition", + "url": "https://example.org/ViewDefinition/conditions", "name": "conditions", "resource": "Condition", "select": [ @@ -107,9 +109,14 @@ curl -s -X POST "http://localhost:8080/fhir/ViewDefinition" \ }' ``` -Note the IDs returned in the responses. Replace `` and -`` in the examples below with these values. You can also -look them up: +Note the IDs returned in the responses. The instance-level `$viewdefinition-run` +examples below use those logical IDs, so replace `` and +`` with them. The `$sqlquery-run` examples reference the views +by their **canonical URL** instead, so replace `` and +`` with the `url` values set above +(`https://example.org/ViewDefinition/patients` and +`https://example.org/ViewDefinition/conditions`). You can look up the stored +resources, including their `url`, with: ```bash curl -s "http://localhost:8080/fhir/ViewDefinition?_count=10" \ @@ -202,8 +209,18 @@ curl -s -X POST "http://localhost:8080/fhir/\$viewdefinition-run" \ The `$sqlquery-run` operation takes a Library resource conforming to the SQLQuery profile. The Library contains Base64-encoded SQL in its `content`, -and references stored ViewDefinitions via `relatedArtifact`. The `label` on -each artifact becomes the table name used in the SQL. +and references stored ViewDefinitions (or SQLViews) via `relatedArtifact`. The +`label` on each artifact becomes the table name used in the SQL. + +Each `relatedArtifact.resource` is the **canonical URL** of a ViewDefinition or +SQLView (optionally suffixed with `|version`), matched against the referenced +resource's `url` element - not its logical id. The placeholders +`` and `` below stand for those canonical +URLs. A ViewDefinition or SQLView must therefore carry a `url` to be +referenceable; ViewDefinitions ingested before URL retention was added must be +re-ingested so their `url` is stored. A reference that is not an absolute +canonical URL is rejected with a 400, and one that matches no stored resource +with a 404. ### Resolving the Library by reference @@ -242,8 +259,7 @@ curl -s -X POST "http://localhost:8080/fhir/\$sqlquery-run?_format=csv" \ ``` If no Library matches, the server responds with 404. If neither (or both) of -`queryResource` and `queryReference` are supplied, the server responds with -400. +`queryResource` and `queryReference` are supplied, the server responds with 400. ### JOIN patients with conditions @@ -276,8 +292,8 @@ curl -s -X POST "http://localhost:8080/fhir/\$sqlquery-run?_format=csv" \ \"data\": \"${SQL_B64}\" }], \"relatedArtifact\": [ - {\"type\": \"depends-on\", \"label\": \"patients\", \"resource\": \"ViewDefinition/\"}, - {\"type\": \"depends-on\", \"label\": \"conditions\", \"resource\": \"ViewDefinition/\"} + {\"type\": \"depends-on\", \"label\": \"patients\", \"resource\": \"\"}, + {\"type\": \"depends-on\", \"label\": \"conditions\", \"resource\": \"\"} ] } }] @@ -326,7 +342,7 @@ curl -s -X POST "http://localhost:8080/fhir/\$sqlquery-run?_format=csv" \ \"data\": \"${SQL_B64}\" }], \"relatedArtifact\": [ - {\"type\": \"depends-on\", \"label\": \"conditions\", \"resource\": \"ViewDefinition/\"} + {\"type\": \"depends-on\", \"label\": \"conditions\", \"resource\": \"\"} ] } }] @@ -374,8 +390,8 @@ curl -s -X POST "http://localhost:8080/fhir/\$sqlquery-run?_format=csv" \ \"data\": \"${SQL_B64}\" }], \"relatedArtifact\": [ - {\"type\": \"depends-on\", \"label\": \"patients\", \"resource\": \"ViewDefinition/\"}, - {\"type\": \"depends-on\", \"label\": \"conditions\", \"resource\": \"ViewDefinition/\"} + {\"type\": \"depends-on\", \"label\": \"patients\", \"resource\": \"\"}, + {\"type\": \"depends-on\", \"label\": \"conditions\", \"resource\": \"\"} ] } }] @@ -432,8 +448,8 @@ curl -s -X POST "http://localhost:8080/fhir/\$sqlquery-run?_format=csv" \ \"data\": \"${SQL_B64}\" }], \"relatedArtifact\": [ - {\"type\": \"depends-on\", \"label\": \"patients\", \"resource\": \"ViewDefinition/\"}, - {\"type\": \"depends-on\", \"label\": \"conditions\", \"resource\": \"ViewDefinition/\"} + {\"type\": \"depends-on\", \"label\": \"patients\", \"resource\": \"\"}, + {\"type\": \"depends-on\", \"label\": \"conditions\", \"resource\": \"\"} ] } }] @@ -462,7 +478,7 @@ curl -s -X POST "http://localhost:8080/fhir/\$sqlquery-run" \ \"data\": \"${SQL_B64}\" }], \"relatedArtifact\": [ - {\"type\": \"depends-on\", \"label\": \"patients\", \"resource\": \"ViewDefinition/\"} + {\"type\": \"depends-on\", \"label\": \"patients\", \"resource\": \"\"} ] } }] @@ -490,7 +506,7 @@ curl -s -X POST "http://localhost:8080/fhir/\$sqlquery-run?_format=json" \ \"data\": \"${SQL_B64}\" }], \"relatedArtifact\": [ - {\"type\": \"depends-on\", \"label\": \"conditions\", \"resource\": \"ViewDefinition/\"} + {\"type\": \"depends-on\", \"label\": \"conditions\", \"resource\": \"\"} ] } }] diff --git a/server/src/test/java/au/csiro/pathling/config/SqlQueryConfigurationTest.java b/server/src/test/java/au/csiro/pathling/config/SqlQueryConfigurationTest.java new file mode 100644 index 0000000000..b41a26016a --- /dev/null +++ b/server/src/test/java/au/csiro/pathling/config/SqlQueryConfigurationTest.java @@ -0,0 +1,82 @@ +/* + * 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.config; + +import static org.assertj.core.api.Assertions.assertThat; + +import jakarta.validation.ConstraintViolation; +import jakarta.validation.Validation; +import jakarta.validation.Validator; +import java.util.Set; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for {@link SqlQueryConfiguration}, covering the {@code maxDependencyDepth} default and + * its {@code @Min(1)} validation. + * + * @author John Grimes + */ +class SqlQueryConfigurationTest { + + private Validator validator; + + @BeforeEach + void setUp() { + validator = Validation.buildDefaultValidatorFactory().getValidator(); + } + + @Test + void defaultMaxDependencyDepthIsTen() { + // The default must be a generous-but-bounded value so that real, shallow view graphs are never + // rejected while pathological fan-out is still capped. + assertThat(new SqlQueryConfiguration().getMaxDependencyDepth()).isEqualTo(10); + } + + @Test + void acceptsPositiveMaxDependencyDepth() { + final SqlQueryConfiguration config = new SqlQueryConfiguration(); + config.setMaxDependencyDepth(1); + + final Set> violations = validator.validate(config); + + assertThat(violations).isEmpty(); + } + + @Test + void rejectsZeroMaxDependencyDepth() { + // A depth of zero would forbid even a single dependency, which is nonsensical for a feature + // whose purpose is dependency resolution. + final SqlQueryConfiguration config = new SqlQueryConfiguration(); + config.setMaxDependencyDepth(0); + + final Set> violations = validator.validate(config); + + assertThat(violations).isNotEmpty(); + } + + @Test + void rejectsNegativeMaxDependencyDepth() { + final SqlQueryConfiguration config = new SqlQueryConfiguration(); + config.setMaxDependencyDepth(-5); + + final Set> violations = validator.validate(config); + + assertThat(violations).isNotEmpty(); + } +} diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/CanonicalReferenceTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/CanonicalReferenceTest.java new file mode 100644 index 0000000000..33dde3fe76 --- /dev/null +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/CanonicalReferenceTest.java @@ -0,0 +1,169 @@ +/* + * 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 org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import java.util.List; +import java.util.function.Function; +import java.util.function.Predicate; +import org.hl7.fhir.r4.model.Enumerations.PublicationStatus; +import org.hl7.fhir.r4.model.Library; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** + * Unit tests for {@link CanonicalReference} covering the url/version split, the canonical-form + * predicate, and candidate selection (exact version, then prefer-active-then-greatest-version). + * + * @author John Grimes + */ +class CanonicalReferenceTest { + + // --------------------------------------------------------------------------- + // Parsing. + // --------------------------------------------------------------------------- + + @Test + void parsesUrlWithoutVersion() { + final CanonicalReference reference = CanonicalReference.parse("https://example.org/V"); + + assertThat(reference.getUrl()).isEqualTo("https://example.org/V"); + assertThat(reference.getVersion()).isNull(); + assertThat(reference.hasVersion()).isFalse(); + } + + @Test + void parsesUrlWithVersion() { + final CanonicalReference reference = CanonicalReference.parse("https://example.org/V|2"); + + assertThat(reference.getUrl()).isEqualTo("https://example.org/V"); + assertThat(reference.getVersion()).isEqualTo("2"); + assertThat(reference.hasVersion()).isTrue(); + } + + @Test + void treatsAnEmptyVersionSuffixAsAbsent() { + final CanonicalReference reference = CanonicalReference.parse("https://example.org/V|"); + + assertThat(reference.getUrl()).isEqualTo("https://example.org/V"); + assertThat(reference.getVersion()).isNull(); + } + + @Test + void rejectsABlankUrlSegment() { + assertThatThrownBy(() -> CanonicalReference.parse("|2")) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining("url"); + } + + // --------------------------------------------------------------------------- + // Canonical-form detection. + // --------------------------------------------------------------------------- + + @ParameterizedTest + @ValueSource( + strings = { + "https://example.org/V", + "http://example.org/V", + "https://example.org/V|2", + "urn:uuid:53fefa32-fcbb-4ff8-8a92-55ee120877b7" + }) + void recognisesAbsoluteCanonicalUrls(final String value) { + assertThat(CanonicalReference.isCanonical(value)).isTrue(); + } + + @ParameterizedTest + @ValueSource( + strings = { + "ViewDefinition/abc", + "Library/abc", + "abc", + "patient-view", + "https://example.org/V#fragment", + "https://example.org/V|1|2", + "" + }) + void rejectsNonCanonicalValues(final String value) { + assertThat(CanonicalReference.isCanonical(value)).isFalse(); + } + + // --------------------------------------------------------------------------- + // Candidate selection. + // --------------------------------------------------------------------------- + + @Test + void selectsTheSoleCandidate() { + final Library only = library("1.0", PublicationStatus.RETIRED); + final CanonicalReference reference = CanonicalReference.parse("https://example.org/V"); + + assertThat(reference.select(List.of(only), isActive(), versionOf())).isSameAs(only); + } + + @Test + void withAVersionReturnsAMatchingCandidateWithoutPreferringStatus() { + // The caller has already filtered to the exact version, so any candidate is acceptable; the + // status preference does not apply when a version was explicitly requested. + final Library first = library("2", PublicationStatus.RETIRED); + final Library second = library("2", PublicationStatus.ACTIVE); + final CanonicalReference reference = CanonicalReference.parse("https://example.org/V|2"); + + assertThat(reference.select(List.of(first, second), isActive(), versionOf())).isSameAs(first); + } + + @Test + void prefersActiveOverRetiredWhenNoVersionSupplied() { + final Library retired = library("3.0", PublicationStatus.RETIRED); + final Library active = library("1.0", PublicationStatus.ACTIVE); + final CanonicalReference reference = CanonicalReference.parse("https://example.org/V"); + + assertThat(reference.select(List.of(retired, active), isActive(), versionOf())) + .isSameAs(active); + } + + @Test + void prefersTheGreatestVersionAmongActiveCandidates() { + final Library v1 = library("1.0", PublicationStatus.ACTIVE); + final Library v2 = library("2.0", PublicationStatus.ACTIVE); + final CanonicalReference reference = CanonicalReference.parse("https://example.org/V"); + + assertThat(reference.select(List.of(v1, v2), isActive(), versionOf())).isSameAs(v2); + } + + // --------------------------------------------------------------------------- + // Helpers. + // --------------------------------------------------------------------------- + + private static Predicate isActive() { + return library -> library.getStatus() == PublicationStatus.ACTIVE; + } + + private static Function versionOf() { + return Library::getVersion; + } + + private static Library library(final String version, final PublicationStatus status) { + final Library library = new Library(); + library.setVersion(version); + library.setStatus(status); + return library; + } +} diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/LibraryReferenceResolverTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/LibraryReferenceResolverTest.java index 426cbabfda..2ab8298896 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/LibraryReferenceResolverTest.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/LibraryReferenceResolverTest.java @@ -68,7 +68,7 @@ void setUp() { readExecutor = mock(ReadExecutor.class); resolver = new LibraryReferenceResolver( - readExecutor, mock(DataSource.class), mock(FhirEncoders.class)); + readExecutor, mock(DataSource.class), mock(FhirEncoders.class), authDisabledConfig()); } @Test @@ -140,7 +140,9 @@ class CanonicalReferences { @BeforeEach void setUp() { dataSource = mock(DataSource.class); - resolver = new LibraryReferenceResolver(mock(ReadExecutor.class), dataSource, fhirEncoders); + resolver = + new LibraryReferenceResolver( + mock(ReadExecutor.class), dataSource, fhirEncoders, authDisabledConfig()); } @Test @@ -239,6 +241,17 @@ private Dataset libraryDataset(final Library... libraries) { // Helpers shared across nested classes. // --------------------------------------------------------------------------- + /** Builds a server configuration with authorisation disabled, so no metadata READ is enforced. */ + private static au.csiro.pathling.config.ServerConfiguration authDisabledConfig() { + final au.csiro.pathling.config.ServerConfiguration config = + new au.csiro.pathling.config.ServerConfiguration(); + final au.csiro.pathling.config.AuthorizationConfiguration auth = + new au.csiro.pathling.config.AuthorizationConfiguration(); + auth.setEnabled(false); + config.setAuth(auth); + return config; + } + private static Library newLibrary( final String id, final String url, final String version, final PublicationStatus status) { final Library library = new Library(); diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/RequestViewResolutionTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/RequestViewResolutionTest.java index f99003b997..e452ab8cca 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/RequestViewResolutionTest.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/RequestViewResolutionTest.java @@ -18,111 +18,167 @@ package au.csiro.pathling.operations.sqlquery; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import au.csiro.pathling.config.AuthorizationConfiguration; import au.csiro.pathling.config.ServerConfiguration; +import au.csiro.pathling.encoders.FhirEncoders; import au.csiro.pathling.encoders.ViewDefinitionResource; import au.csiro.pathling.encoders.ViewDefinitionResource.ColumnComponent; import au.csiro.pathling.encoders.ViewDefinitionResource.SelectComponent; -import au.csiro.pathling.read.ReadExecutor; -import au.csiro.pathling.views.FhirView; +import au.csiro.pathling.io.source.DataSource; +import au.csiro.pathling.library.io.source.QueryableDataSource; +import au.csiro.pathling.operations.view.ViewExecutionHelper; import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import jakarta.annotation.Nonnull; -import java.util.List; import java.util.Map; +import java.util.Optional; +import java.util.Set; import org.hl7.fhir.r4.model.CodeType; +import org.hl7.fhir.r4.model.Parameters; +import org.hl7.fhir.r4.model.Parameters.ParametersParameterComponent; import org.hl7.fhir.r4.model.StringType; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; /** - * Unit tests for the request-supplied view resolution path of {@link ViewResolver}: a supplied view - * is matched to a {@code relatedArtifact} reference by id and preferred over server storage, while - * a reference with no matching supplied view falls back to storage. + * Tests for the resolution of request-supplied views against canonical dependency references: a + * supplied view is matched to a reference by its {@code url} and preferred over storage (resolved + * by {@link ViewResolver}), while a supplied view that carries no {@code url} is rejected at parse + * time (by {@link SqlQueryExportRequestParser}). * * @author John Grimes */ class RequestViewResolutionTest { - private ReadExecutor readExecutor; - private ViewResolver resolver; - - @BeforeEach - void setUp() { - readExecutor = mock(ReadExecutor.class); - final ServerConfiguration serverConfiguration = new ServerConfiguration(); - final AuthorizationConfiguration auth = new AuthorizationConfiguration(); - auth.setEnabled(false); - serverConfiguration.setAuth(auth); - resolver = new ViewResolver(readExecutor, serverConfiguration, FhirContext.forR4()); + private static final String PATIENTS_URL = "https://example.org/Patients"; + + // --------------------------------------------------------------------------- + // Supplied-view matching by url (ViewResolver). + // --------------------------------------------------------------------------- + + @Nested + class SuppliedViewMatching { + + private ViewResolver resolver; + + @BeforeEach + void setUp() { + final ServerConfiguration serverConfiguration = new ServerConfiguration(); + final AuthorizationConfiguration auth = new AuthorizationConfiguration(); + auth.setEnabled(false); + serverConfiguration.setAuth(auth); + resolver = + new ViewResolver( + mock(DataSource.class), + mock(FhirEncoders.class), + serverConfiguration, + FhirContext.forR4Cached()); + } + + @Test + void suppliedViewIsMatchedByUrlAndPreferredOverStorage() { + final var supplied = + au.csiro.pathling.views.FhirView.ofResource("Patient") + .select( + au.csiro.pathling.views.FhirView.columns( + au.csiro.pathling.views.FhirView.column("id", "id"))) + .build(); + + final Optional resolved = + resolver.resolveSuppliedView( + new ViewArtifactReference("patients", PATIENTS_URL), Map.of(PATIENTS_URL, supplied)); + + assertThat(resolved).isPresent(); + assertThat(resolved.get().getView()).isSameAs(supplied); + assertThat(resolved.get().getCanonicalKey()).isEqualTo(PATIENTS_URL); + } + + @Test + void resolvesNoSuppliedViewWhenNoneMatchesTheUrl() { + final Optional resolved = + resolver.resolveSuppliedView( + new ViewArtifactReference("patients", PATIENTS_URL), Map.of()); + + assertThat(resolved).isEmpty(); + } } - @Test - void suppliedViewIsPreferredAndStorageIsNotConsulted() { - final FhirView supplied = - FhirView.ofResource("Patient") - .select(FhirView.columns(FhirView.column("id", "id"))) - .build(); - final Map suppliedViews = Map.of("patient-bp", supplied); - - final Map resolved = - resolver.resolve( - List.of(new ViewArtifactReference("patients", "ViewDefinition/patient-bp")), - suppliedViews); - - assertThat(resolved.get("patients")).isSameAs(supplied); - verify(readExecutor, never()) - .read( - org.mockito.ArgumentMatchers.eq("ViewDefinition"), - org.mockito.ArgumentMatchers.anyString()); + // --------------------------------------------------------------------------- + // Url-less supplied-view rejection (SqlQueryExportRequestParser). + // --------------------------------------------------------------------------- + + @Nested + class SuppliedViewValidation { + + private ViewExecutionHelper viewExecutionHelper; + private SqlQueryExportRequestParser parser; + + @BeforeEach + void setUp() { + viewExecutionHelper = mock(ViewExecutionHelper.class); + final ServerConfiguration serverConfiguration = new ServerConfiguration(); + final AuthorizationConfiguration auth = new AuthorizationConfiguration(); + auth.setEnabled(false); + serverConfiguration.setAuth(auth); + parser = + new SqlQueryExportRequestParser( + mock(SqlQueryPipeline.class), + mock(LibraryReferenceResolver.class), + viewExecutionHelper, + FhirContext.forR4Cached(), + serverConfiguration, + mock(QueryableDataSource.class)); + } + + @Test + void rejectsASuppliedViewWithoutAUrl() { + // The resolved view has no url, so it can never satisfy a canonical reference. + when(viewExecutionHelper.resolveViewInput(any(), any())) + .thenReturn(viewDefinitionWithoutUrl()); + final Parameters body = parametersWithInlineView(); + + assertThatThrownBy( + () -> + parser.parse(requestDetails(body), null, null, null, null, Set.of(), null, null)) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining("url"); + } + + @Nonnull + private Parameters parametersWithInlineView() { + final Parameters parameters = new Parameters(); + final ParametersParameterComponent view = parameters.addParameter().setName("view"); + view.addPart().setName("viewResource").setResource(viewDefinitionWithoutUrl()); + return parameters; + } + + @Nonnull + private ServletRequestDetails requestDetails(@Nonnull final Parameters body) { + final ServletRequestDetails requestDetails = mock(ServletRequestDetails.class); + when(requestDetails.getResource()).thenReturn(body); + when(requestDetails.getCompleteUrl()).thenReturn("http://localhost/fhir/$sqlquery-export"); + when(requestDetails.getFhirServerBase()).thenReturn("http://localhost/fhir"); + return requestDetails; + } } - @Test - void referenceWithNoSuppliedViewFallsBackToStorage() { - when(readExecutor.read("ViewDefinition", "patient-bp")) - .thenReturn(simpleViewDefinition("patient-bp", "Patient")); - - final Map resolved = - resolver.resolve( - List.of(new ViewArtifactReference("patients", "ViewDefinition/patient-bp")), Map.of()); - - assertThat(resolved).containsOnlyKeys("patients"); - verify(readExecutor).read("ViewDefinition", "patient-bp"); - } - - @Test - void mixesSuppliedAndStorageResolvedViews() { - final FhirView supplied = - FhirView.ofResource("Patient") - .select(FhirView.columns(FhirView.column("id", "id"))) - .build(); - when(readExecutor.read("ViewDefinition", "obs-view")) - .thenReturn(simpleViewDefinition("obs-view", "Observation")); - - final Map resolved = - resolver.resolve( - List.of( - new ViewArtifactReference("patients", "ViewDefinition/patient-bp"), - new ViewArtifactReference("obs", "ViewDefinition/obs-view")), - Map.of("patient-bp", supplied)); - - assertThat(resolved.get("patients")).isSameAs(supplied); - assertThat(resolved.get("obs").getResource()).isEqualTo("Observation"); - verify(readExecutor).read("ViewDefinition", "obs-view"); - verify(readExecutor, never()).read("ViewDefinition", "patient-bp"); - } + // --------------------------------------------------------------------------- + // Helpers. + // --------------------------------------------------------------------------- @Nonnull - private static ViewDefinitionResource simpleViewDefinition( - @Nonnull final String id, @Nonnull final String resourceType) { + private static ViewDefinitionResource viewDefinitionWithoutUrl() { final ViewDefinitionResource view = new ViewDefinitionResource(); - view.setId(id); - view.setName(new StringType(id + "_view")); - view.setResource(new CodeType(resourceType)); + view.setId("no-url-view"); + view.setName(new StringType("no_url_view")); + view.setResource(new CodeType("Patient")); view.setStatus(new CodeType("active")); final SelectComponent select = new SelectComponent(); final ColumnComponent column = new ColumnComponent(); diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlDependencyResolverTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlDependencyResolverTest.java new file mode 100644 index 0000000000..7b13d85076 --- /dev/null +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlDependencyResolverTest.java @@ -0,0 +1,358 @@ +/* + * 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 org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import au.csiro.pathling.config.AuthorizationConfiguration; +import au.csiro.pathling.config.ServerConfiguration; +import au.csiro.pathling.config.SqlQueryConfiguration; +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.List; +import java.util.Map; +import java.util.Optional; +import org.hl7.fhir.r4.model.Library; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for {@link SqlDependencyResolver} covering canonical-URL resolution, the resolved + * graph shape for a {@code SQLQuery -> SQLView -> ViewDefinition} chain, request-supplied view + * preference, diamond de-duplication (including bare-url vs {@code url|version}), and the + * structural rejections (cycles, depth, ambiguity, not-found, and wrong-typed dependencies). + * + * @author John Grimes + */ +class SqlDependencyResolverTest { + + private static final String PATIENT_VIEW_URL = + SqlLibraryFixtures.viewDefinitionUrl("patient-view"); + + private ViewResolver viewResolver; + private LibraryReferenceResolver libraryReferenceResolver; + private ServerConfiguration serverConfiguration; + private SqlDependencyResolver resolver; + + @BeforeEach + void setUp() { + viewResolver = mock(ViewResolver.class); + libraryReferenceResolver = mock(LibraryReferenceResolver.class); + serverConfiguration = new ServerConfiguration(); + final AuthorizationConfiguration auth = new AuthorizationConfiguration(); + auth.setEnabled(false); + serverConfiguration.setAuth(auth); + serverConfiguration.setSqlQuery(new SqlQueryConfiguration()); + resolver = + new SqlDependencyResolver( + viewResolver, libraryReferenceResolver, new SqlLibraryParser(), serverConfiguration); + } + + // --------------------------------------------------------------------------- + // Canonical-URL resolution (US1). + // --------------------------------------------------------------------------- + + @Test + void resolvesAViewDefinitionByUrl() { + stubStoredViewDefinition(PATIENT_VIEW_URL, PATIENT_VIEW_URL, "Patient"); + + final ResolvedDependencyGraph graph = + resolver.resolve(sqlQuery("SELECT * FROM p", "p", PATIENT_VIEW_URL), Map.of()); + + assertThat(graph.getTopLevelKeysByLabel()).containsEntry("p", PATIENT_VIEW_URL); + assertThat(graph.getNodesByKey().get(PATIENT_VIEW_URL)) + .isInstanceOf(ResolvedViewDefinition.class); + } + + @Test + void resolvesASqlViewByUrl() { + final String baseUrl = SqlLibraryFixtures.sqlViewUrl("base"); + stubSqlView(baseUrl, "SELECT * FROM pv", "pv", PATIENT_VIEW_URL); + stubStoredViewDefinition(PATIENT_VIEW_URL, PATIENT_VIEW_URL, "Patient"); + + final ResolvedDependencyGraph graph = + resolver.resolve(sqlQuery("SELECT * FROM b", "b", baseUrl), Map.of()); + + assertThat(graph.getTopLevelKeysByLabel()).containsEntry("b", baseUrl); + assertThat(graph.getNodesByKey().get(baseUrl)).isInstanceOf(ResolvedSqlView.class); + assertThat(graph.getNodesByKey().get(PATIENT_VIEW_URL)) + .isInstanceOf(ResolvedViewDefinition.class); + } + + @Test + void recursesThroughASqlViewUrlDependencies() { + // SQLQuery -> v1 (SQLView) -> v2 (SQLView) -> ViewDefinition, all by canonical URL. + final String v1Url = SqlLibraryFixtures.sqlViewUrl("v1"); + final String v2Url = SqlLibraryFixtures.sqlViewUrl("v2"); + stubSqlView(v1Url, "SELECT * FROM x", "x", v2Url); + stubSqlView(v2Url, "SELECT * FROM pv", "pv", PATIENT_VIEW_URL); + stubStoredViewDefinition(PATIENT_VIEW_URL, PATIENT_VIEW_URL, "Patient"); + + final ResolvedDependencyGraph graph = + resolver.resolve(sqlQuery("SELECT * FROM v", "v", v1Url), Map.of()); + + assertThat(graph.getOrderedNodes()).hasSize(3); + // Dependencies precede dependents: VD, then v2, then v1. + assertThat(graph.getOrderedNodes().get(0).getCanonicalKey()).isEqualTo(PATIENT_VIEW_URL); + assertThat(graph.getOrderedNodes().get(1).getCanonicalKey()).isEqualTo(v2Url); + assertThat(graph.getOrderedNodes().get(2).getCanonicalKey()).isEqualTo(v1Url); + } + + @Test + void buildsTopologicallyOrderedTwoNodeGraph() { + final String baseUrl = SqlLibraryFixtures.sqlViewUrl("base"); + stubSqlView(baseUrl, "SELECT * FROM pv", "pv", PATIENT_VIEW_URL); + stubStoredViewDefinition(PATIENT_VIEW_URL, PATIENT_VIEW_URL, "Patient"); + + final ResolvedDependencyGraph graph = + resolver.resolve(sqlQuery("SELECT * FROM b", "b", baseUrl), Map.of()); + + assertThat(graph.getOrderedNodes()).hasSize(2); + assertThat(graph.getOrderedNodes().get(0).getCanonicalKey()).isEqualTo(PATIENT_VIEW_URL); + assertThat(graph.getOrderedNodes().get(1).getCanonicalKey()).isEqualTo(baseUrl); + + final ResolvedSqlView sqlView = (ResolvedSqlView) graph.getNodesByKey().get(baseUrl); + assertThat(sqlView.getChildKeysByLabel()).containsEntry("pv", PATIENT_VIEW_URL); + } + + @Test + void deduplicatesABareUrlAndAVersionedReferenceToTheSameResource() { + // Both the bare url and url|2 resolve to the same stored ViewDefinition (version 2), so they + // normalise to the same canonical key and materialise once. + final String versionedKey = PATIENT_VIEW_URL + "|2"; + stubStoredViewDefinition(PATIENT_VIEW_URL, versionedKey, "Patient"); + stubStoredViewDefinition(PATIENT_VIEW_URL + "|2", versionedKey, "Patient"); + + final ResolvedDependencyGraph graph = + resolver.resolve( + sqlQueryWithDeps( + "SELECT * FROM a JOIN b", + Map.of("a", PATIENT_VIEW_URL, "b", PATIENT_VIEW_URL + "|2")), + Map.of()); + + assertThat(graph.getOrderedNodes()).hasSize(1); + assertThat(graph.getNodesByKey()).containsKey(versionedKey); + assertThat(graph.getTopLevelKeysByLabel()) + .containsEntry("a", versionedKey) + .containsEntry("b", versionedKey); + } + + @Test + void resolvesADiamondSharedNodeOnce() { + final String leftUrl = SqlLibraryFixtures.sqlViewUrl("left"); + final String rightUrl = SqlLibraryFixtures.sqlViewUrl("right"); + final String sharedUrl = SqlLibraryFixtures.sqlViewUrl("shared"); + stubSqlView(leftUrl, "SELECT * FROM s", "s", sharedUrl); + stubSqlView(rightUrl, "SELECT * FROM s", "s", sharedUrl); + stubSqlView(sharedUrl, "SELECT * FROM pv", "pv", PATIENT_VIEW_URL); + stubStoredViewDefinition(PATIENT_VIEW_URL, PATIENT_VIEW_URL, "Patient"); + + final ResolvedDependencyGraph graph = + resolver.resolve( + sqlQueryWithDeps("SELECT * FROM l JOIN r", Map.of("l", leftUrl, "r", rightUrl)), + Map.of()); + + final long sharedCount = + graph.getOrderedNodes().stream() + .filter(node -> sharedUrl.equals(node.getCanonicalKey())) + .count(); + assertThat(sharedCount).isEqualTo(1); + assertThat(graph.getOrderedNodes()).hasSize(4); // shared, vd, left, right. + } + + @Test + void resolvesTheSameLabelInDifferentNodesWithoutCollision() { + final String v1Url = SqlLibraryFixtures.sqlViewUrl("v1"); + final String v2Url = SqlLibraryFixtures.sqlViewUrl("v2"); + final String aUrl = SqlLibraryFixtures.viewDefinitionUrl("a"); + final String bUrl = SqlLibraryFixtures.viewDefinitionUrl("b"); + stubSqlView(v1Url, "SELECT * FROM t", "t", aUrl); + stubSqlView(v2Url, "SELECT * FROM t", "t", bUrl); + stubStoredViewDefinition(aUrl, aUrl, "Patient"); + stubStoredViewDefinition(bUrl, bUrl, "Observation"); + + final ResolvedDependencyGraph graph = + resolver.resolve( + sqlQueryWithDeps("SELECT * FROM one, two", Map.of("one", v1Url, "two", v2Url)), + Map.of()); + + final ResolvedSqlView v1 = (ResolvedSqlView) graph.getNodesByKey().get(v1Url); + final ResolvedSqlView v2 = (ResolvedSqlView) graph.getNodesByKey().get(v2Url); + assertThat(v1.getChildKeysByLabel()).containsEntry("t", aUrl); + assertThat(v2.getChildKeysByLabel()).containsEntry("t", bUrl); + } + + @Test + void prefersARequestSuppliedViewOverStorage() { + final FhirView supplied = fhirView("Patient"); + when(viewResolver.resolveSuppliedView( + argThat(ref -> ref != null && PATIENT_VIEW_URL.equals(ref.getCanonicalUrl())), + argThat(map -> map.containsKey(PATIENT_VIEW_URL)))) + .thenReturn(Optional.of(new ResolvedViewDefinition(PATIENT_VIEW_URL, supplied))); + + final ResolvedDependencyGraph graph = + resolver.resolve( + sqlQuery("SELECT * FROM p", "p", PATIENT_VIEW_URL), Map.of(PATIENT_VIEW_URL, supplied)); + + final ResolvedViewDefinition node = + (ResolvedViewDefinition) graph.getNodesByKey().get(PATIENT_VIEW_URL); + assertThat(node.getView()).isSameAs(supplied); + } + + // --------------------------------------------------------------------------- + // Cycles and depth (keyed by canonical identity). + // --------------------------------------------------------------------------- + + @Test + void rejectsACycleNamingTheChain() { + final String aUrl = SqlLibraryFixtures.sqlViewUrl("a"); + final String bUrl = SqlLibraryFixtures.sqlViewUrl("b"); + stubSqlView(aUrl, "SELECT * FROM b", "b", bUrl); + stubSqlView(bUrl, "SELECT * FROM a", "a", aUrl); + + assertThatThrownBy(() -> resolver.resolve(sqlQuery("SELECT * FROM x", "x", aUrl), Map.of())) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContainingAll("Cyclic", aUrl, bUrl); + } + + @Test + void rejectsASelfReference() { + final String selfUrl = SqlLibraryFixtures.sqlViewUrl("self"); + stubSqlView(selfUrl, "SELECT * FROM s", "s", selfUrl); + + assertThatThrownBy(() -> resolver.resolve(sqlQuery("SELECT * FROM x", "x", selfUrl), Map.of())) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining("Cyclic"); + } + + @Test + void rejectsAGraphDeeperThanTheConfiguredLimit() { + serverConfiguration.getSqlQuery().setMaxDependencyDepth(2); + final String v1Url = SqlLibraryFixtures.sqlViewUrl("v1"); + final String v2Url = SqlLibraryFixtures.sqlViewUrl("v2"); + final String v3Url = SqlLibraryFixtures.sqlViewUrl("v3"); + stubSqlView(v1Url, "SELECT * FROM x", "x", v2Url); + stubSqlView(v2Url, "SELECT * FROM y", "y", v3Url); + stubSqlView(v3Url, "SELECT * FROM pv", "pv", PATIENT_VIEW_URL); + stubStoredViewDefinition(PATIENT_VIEW_URL, PATIENT_VIEW_URL, "Patient"); + + assertThatThrownBy(() -> resolver.resolve(sqlQuery("SELECT * FROM v", "v", v1Url), Map.of())) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContainingAll("deeper", "2"); + } + + // --------------------------------------------------------------------------- + // Error surface (US2): not-found, ambiguity, wrong-typed dependency. + // --------------------------------------------------------------------------- + + @Test + void reportsNotFoundWhenNeitherAViewDefinitionNorASqlViewMatches() { + final String missingUrl = SqlLibraryFixtures.viewDefinitionUrl("missing"); + + assertThatThrownBy(() -> resolver.resolve(sqlQuery("SELECT 1", "x", missingUrl), Map.of())) + .isInstanceOf(ResourceNotFoundException.class) + .hasMessageContaining("x") + .hasMessageContaining(missingUrl); + } + + @Test + void rejectsAnAmbiguousReferenceMatchingBothTypes() { + final String clashUrl = SqlLibraryFixtures.viewDefinitionUrl("clash"); + stubStoredViewDefinition(clashUrl, clashUrl, "Patient"); + stubSqlView(clashUrl, "SELECT 1", "p", PATIENT_VIEW_URL); + + assertThatThrownBy(() -> resolver.resolve(sqlQuery("SELECT 1", "c", clashUrl), Map.of())) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining("ambiguous") + .hasMessageContaining("c") + .hasMessageContaining(clashUrl); + } + + @Test + void rejectsASqlQueryReferencedAsADependency() { + // A Library that is itself a sql-query (not a sql-view) cannot be a dependency. + final String queryUrl = SqlLibraryFixtures.sqlViewUrl("q"); + final Library nested = SqlLibraryFixtures.sqlQuery("SELECT 1"); + nested.setUrl(queryUrl); + when(libraryReferenceResolver.tryResolveSqlViewLibrary(queryUrl)) + .thenReturn(Optional.of(nested)); + + assertThatThrownBy(() -> resolver.resolve(sqlQuery("SELECT 1", "q", queryUrl), Map.of())) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining("sql-query") + .hasMessageContaining("SQLView"); + } + + // --------------------------------------------------------------------------- + // Helpers. + // --------------------------------------------------------------------------- + + /** Builds a top-level SQLQuery ParsedSqlQuery with one dependency. */ + @Nonnull + private static ParsedSqlQuery sqlQuery( + @Nonnull final String sql, @Nonnull final String label, @Nonnull final String resource) { + return sqlQueryWithDeps(sql, Map.of(label, resource)); + } + + /** Builds a top-level SQLQuery ParsedSqlQuery with several dependencies. */ + @Nonnull + private static ParsedSqlQuery sqlQueryWithDeps( + @Nonnull final String sql, @Nonnull final Map dependenciesByLabel) { + final List references = new ArrayList<>(); + dependenciesByLabel.forEach( + (label, resource) -> references.add(new ViewArtifactReference(label, resource))); + return new ParsedSqlQuery(sql, references, List.of(), SqlLibraryParser.SQL_QUERY_TYPE_CODE); + } + + /** + * Stubs the view resolver to resolve a reference whose canonical url matches {@code referenceUrl} + * to a stored ViewDefinition over the given resource type, with the given resolved canonical key. + */ + private void stubStoredViewDefinition( + @Nonnull final String referenceUrl, + @Nonnull final String resolvedKey, + @Nonnull final String resourceType) { + when(viewResolver.resolveStoredViewDefinition( + argThat(ref -> ref != null && referenceUrl.equals(ref.getCanonicalUrl())))) + .thenReturn(Optional.of(new ResolvedViewDefinition(resolvedKey, fhirView(resourceType)))); + } + + /** Stubs the library resolver to return a stored SQLView (carrying {@code url}) with one dep. */ + private void stubSqlView( + @Nonnull final String url, + @Nonnull final String sql, + @Nonnull final String depLabel, + @Nonnull final String depResource) { + final Library sqlView = SqlLibraryFixtures.sqlViewWithUrl(url, sql, depLabel, depResource); + when(libraryReferenceResolver.tryResolveSqlViewLibrary(url)).thenReturn(Optional.of(sqlView)); + } + + @Nonnull + private static FhirView fhirView(@Nonnull final String resourceType) { + return FhirView.ofResource(resourceType) + .select(FhirView.columns(FhirView.column("id", "id"))) + .build(); + } +} diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlLibraryFixtures.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlLibraryFixtures.java new file mode 100644 index 0000000000..96321e663f --- /dev/null +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlLibraryFixtures.java @@ -0,0 +1,235 @@ +/* + * 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.operations.sqlquery.SqlLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.SQL_QUERY_TYPE_CODE; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.SQL_VIEW_TYPE_CODE; + +import jakarta.annotation.Nonnull; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import org.hl7.fhir.r4.model.Attachment; +import org.hl7.fhir.r4.model.CodeableConcept; +import org.hl7.fhir.r4.model.Coding; +import org.hl7.fhir.r4.model.Enumerations.PublicationStatus; +import org.hl7.fhir.r4.model.Library; +import org.hl7.fhir.r4.model.ParameterDefinition; +import org.hl7.fhir.r4.model.ParameterDefinition.ParameterUse; +import org.hl7.fhir.r4.model.RelatedArtifact; +import org.hl7.fhir.r4.model.RelatedArtifact.RelatedArtifactType; + +/** + * Test fixtures for constructing {@code SQLQuery} and {@code SQLView} {@link Library} resources. + * The builders mirror the SQL on FHIR profiles: a {@code Library.type} coding, one {@code + * application/sql} content entry carrying the Base64-encoded SQL, {@code depends-on} {@code + * relatedArtifact} dependencies (label plus resource reference), and, for {@code SQLQuery}, + * optional input parameters. + * + * @author John Grimes + */ +public final class SqlLibraryFixtures { + + /** Base canonical URL under which test ViewDefinitions and SQLViews are published. */ + public static final String CANONICAL_BASE = "https://pathling.csiro.au/test/"; + + private SqlLibraryFixtures() { + // Utility class. + } + + /** + * Builds the canonical URL for a test ViewDefinition with the given local name. The final segment + * is deliberately the local name so tests can give a ViewDefinition a logical id that differs + * from the URL's final segment. + * + * @param name the local name segment + * @return the canonical URL + */ + @Nonnull + public static String viewDefinitionUrl(@Nonnull final String name) { + return CANONICAL_BASE + "ViewDefinition/" + name; + } + + /** + * Builds the canonical URL for a test SQLView Library with the given local name. + * + * @param name the local name segment + * @return the canonical URL + */ + @Nonnull + public static String sqlViewUrl(@Nonnull final String name) { + return CANONICAL_BASE + "Library/" + name; + } + + /** + * Builds a {@code SQLView} Library that carries its own canonical {@code url} and a single {@code + * depends-on} dependency referenced by canonical URL. + * + * @param url the SQLView's canonical url, against which dependency references resolve it + * @param sql the SQL text to embed + * @param label the dependency table label + * @param resource the dependency resource reference (a canonical URL) + * @return a {@code SQLView} Library carrying a url and one dependency + */ + @Nonnull + public static Library sqlViewWithUrl( + @Nonnull final String url, + @Nonnull final String sql, + @Nonnull final String label, + @Nonnull final String resource) { + final Library library = sqlView(sql, label, resource); + library.setUrl(url); + return library; + } + + /** + * Builds a {@code SQLView} Library that carries its own canonical {@code url} and a set of {@code + * depends-on} dependencies referenced by canonical URL. + * + * @param url the SQLView's canonical url, against which dependency references resolve it + * @param sql the SQL text to embed + * @param dependenciesByLabel the dependencies keyed by table label, each value a canonical URL + * @return a {@code SQLView} Library carrying a url and the given dependencies + */ + @Nonnull + public static Library sqlViewWithUrl( + @Nonnull final String url, + @Nonnull final String sql, + @Nonnull final Map dependenciesByLabel) { + final Library library = sqlView(sql, dependenciesByLabel); + library.setUrl(url); + return library; + } + + /** + * Builds a {@code SQLQuery} Library carrying the given SQL, with no dependencies or parameters. + * + * @param sql the SQL text to embed as Base64 {@code application/sql} content + * @return a minimal {@code SQLQuery} Library + */ + @Nonnull + public static Library sqlQuery(@Nonnull final String sql) { + return baseLibrary(SQL_QUERY_TYPE_CODE, sql); + } + + /** + * Builds a {@code SQLQuery} Library carrying the given SQL and a single {@code depends-on} + * dependency. + * + * @param sql the SQL text to embed + * @param label the dependency table label + * @param resource the dependency resource reference + * @return a {@code SQLQuery} Library with one dependency + */ + @Nonnull + public static Library sqlQuery( + @Nonnull final String sql, @Nonnull final String label, @Nonnull final String resource) { + final Library library = baseLibrary(SQL_QUERY_TYPE_CODE, sql); + addDependency(library, label, resource); + return library; + } + + /** + * Builds a {@code SQLView} Library carrying the given SQL, with no dependencies or parameters. + * + * @param sql the SQL text to embed as Base64 {@code application/sql} content + * @return a minimal {@code SQLView} Library + */ + @Nonnull + public static Library sqlView(@Nonnull final String sql) { + return baseLibrary(SQL_VIEW_TYPE_CODE, sql); + } + + /** + * Builds a {@code SQLView} Library carrying the given SQL and a single {@code depends-on} + * dependency. + * + * @param sql the SQL text to embed + * @param label the dependency table label + * @param resource the dependency resource reference + * @return a {@code SQLView} Library with one dependency + */ + @Nonnull + public static Library sqlView( + @Nonnull final String sql, @Nonnull final String label, @Nonnull final String resource) { + final Library library = baseLibrary(SQL_VIEW_TYPE_CODE, sql); + addDependency(library, label, resource); + return library; + } + + /** + * Builds a {@code SQLView} Library carrying the given SQL and a set of {@code depends-on} + * dependencies, preserving the iteration order of the supplied map. + * + * @param sql the SQL text to embed + * @param dependenciesByLabel the dependencies keyed by table label, each value a resource + * reference + * @return a {@code SQLView} Library with the given dependencies + */ + @Nonnull + public static Library sqlView( + @Nonnull final String sql, @Nonnull final Map dependenciesByLabel) { + final Library library = baseLibrary(SQL_VIEW_TYPE_CODE, sql); + dependenciesByLabel.forEach((label, resource) -> addDependency(library, label, resource)); + return library; + } + + /** + * Adds a {@code depends-on} {@code relatedArtifact} to the given Library. + * + * @param library the Library to extend + * @param label the dependency table label + * @param resource the dependency resource reference + */ + public static void addDependency( + @Nonnull final Library library, @Nonnull final String label, @Nonnull final String resource) { + library.addRelatedArtifact( + new RelatedArtifact() + .setType(RelatedArtifactType.DEPENDSON) + .setLabel(label) + .setResource(resource)); + } + + /** + * Adds an input parameter declaration to the given Library. + * + * @param library the Library to extend + * @param name the parameter name + * @param type the FHIR primitive type code + */ + public static void addParameter( + @Nonnull final Library library, @Nonnull final String name, @Nonnull final String type) { + library.addParameter( + new ParameterDefinition().setName(name).setType(type).setUse(ParameterUse.IN)); + } + + /** Builds a Library with the given SQL on FHIR library-type code and embedded SQL content. */ + @Nonnull + private static Library baseLibrary(@Nonnull final String typeCode, @Nonnull final String sql) { + final Library library = new Library(); + library.setStatus(PublicationStatus.ACTIVE); + library.setType( + new CodeableConcept() + .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(typeCode))); + final Attachment content = new Attachment(); + content.setContentType("application/sql"); + content.setData(sql.getBytes(StandardCharsets.UTF_8)); + library.addContent(content); + return library; + } +} diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryLibraryParserTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlLibraryParserTest.java similarity index 69% rename from server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryLibraryParserTest.java rename to server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlLibraryParserTest.java index 4f0a1b434e..4b7ba2a0a6 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryLibraryParserTest.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlLibraryParserTest.java @@ -17,6 +17,9 @@ package au.csiro.pathling.operations.sqlquery; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.SQL_QUERY_TYPE_CODE; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.SQL_VIEW_TYPE_CODE; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -30,18 +33,17 @@ import org.hl7.fhir.r4.model.RelatedArtifact.RelatedArtifactType; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; -/** Unit tests for {@link SqlQueryLibraryParser}. */ -class SqlQueryLibraryParserTest { +/** Unit tests for {@link SqlLibraryParser} covering both the SQLQuery and SQLView profiles. */ +class SqlLibraryParserTest { - private static final String LIBRARY_TYPE_SYSTEM = - "https://sql-on-fhir.org/ig/CodeSystem/LibraryTypesCodes"; - - private SqlQueryLibraryParser parser; + private SqlLibraryParser parser; @BeforeEach void setUp() { - parser = new SqlQueryLibraryParser(); + parser = new SqlLibraryParser(); } @Test @@ -58,22 +60,22 @@ void extractsViewReferences() { new RelatedArtifact() .setType(RelatedArtifactType.DEPENDSON) .setLabel("patients") - .setResource("ViewDefinition/patient-view")); + .setResource("https://example.org/ViewDefinition/patient-view")); library.addRelatedArtifact( new RelatedArtifact() .setType(RelatedArtifactType.DEPENDSON) .setLabel("observations") - .setResource("ViewDefinition/obs-view")); + .setResource("https://example.org/ViewDefinition/obs-view")); final ParsedSqlQuery result = parser.parse(library); assertThat(result.getViewReferences()).hasSize(2); assertThat(result.getViewReferences().get(0).getLabel()).isEqualTo("patients"); assertThat(result.getViewReferences().get(0).getCanonicalUrl()) - .isEqualTo("ViewDefinition/patient-view"); + .isEqualTo("https://example.org/ViewDefinition/patient-view"); assertThat(result.getViewReferences().get(1).getLabel()).isEqualTo("observations"); assertThat(result.getViewReferences().get(1).getCanonicalUrl()) - .isEqualTo("ViewDefinition/obs-view"); + .isEqualTo("https://example.org/ViewDefinition/obs-view"); } @Test @@ -98,6 +100,63 @@ void handlesEmptyViewReferencesAndParameters() { assertThat(result.getDeclaredParameters()).isEmpty(); } + @Test + void reportsSqlQueryAsNotAView() { + final ParsedSqlQuery result = parser.parse(createMinimalLibrary("SELECT 1")); + assertThat(result.getLibraryTypeCode()).isEqualTo(SQL_QUERY_TYPE_CODE); + assertThat(result.isView()).isFalse(); + } + + // --------------------------------------------------------------------------- + // SQLView profile. + // --------------------------------------------------------------------------- + + @Test + void parsesSqlViewWithTypeCodeSqlAndDependencies() { + final Library library = SqlLibraryFixtures.sqlView("SELECT * FROM patient_view"); + SqlLibraryFixtures.addDependency( + library, "patient_view", "https://example.org/ViewDefinition/patient-view"); + SqlLibraryFixtures.addDependency(library, "base", "https://example.org/Library/base"); + + final ParsedSqlQuery result = parser.parse(library); + + assertThat(result.getLibraryTypeCode()).isEqualTo(SQL_VIEW_TYPE_CODE); + assertThat(result.isView()).isTrue(); + assertThat(result.getSql()).isEqualTo("SELECT * FROM patient_view"); + assertThat(result.getViewReferences()).hasSize(2); + assertThat(result.getViewReferences().get(0).getLabel()).isEqualTo("patient_view"); + assertThat(result.getViewReferences().get(0).getCanonicalUrl()) + .isEqualTo("https://example.org/ViewDefinition/patient-view"); + assertThat(result.getViewReferences().get(1).getCanonicalUrl()) + .isEqualTo("https://example.org/Library/base"); + assertThat(result.getDeclaredParameters()).isEmpty(); + } + + @Test + void rejectsSqlViewDeclaringAParameter() { + // A SQLView SHALL NOT declare parameters; doing so is a 400. + final Library library = SqlLibraryFixtures.sqlView("SELECT 1"); + SqlLibraryFixtures.addParameter(library, "min_age", "integer"); + + assertThatThrownBy(() -> parser.parse(library)) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining(SQL_VIEW_TYPE_CODE) + .hasMessageContaining("parameter"); + } + + @Test + void stillParsesSqlQueryWithParameters() { + // The shared parser must continue to accept SQLQuery parameters unchanged. + final Library library = SqlLibraryFixtures.sqlQuery("SELECT * FROM t WHERE age > :min_age"); + SqlLibraryFixtures.addParameter(library, "min_age", "integer"); + + final ParsedSqlQuery result = parser.parse(library); + + assertThat(result.isView()).isFalse(); + assertThat(result.getDeclaredParameters()).hasSize(1); + assertThat(result.getDeclaredParameters().get(0).getName()).isEqualTo("min_age"); + } + // --------------------------------------------------------------------------- // SQL content rejections. // --------------------------------------------------------------------------- @@ -156,7 +215,7 @@ void rejectsLibraryWithoutTypeCoding() { assertThatThrownBy(() -> parser.parse(library)) .isInstanceOf(InvalidRequestException.class) .hasMessageContaining("Library.type") - .hasMessageContaining("sql-query"); + .hasMessageContaining(SQL_QUERY_TYPE_CODE); } @Test @@ -177,7 +236,7 @@ void rejectsLibraryWithUnrelatedTypeCoding() { assertThatThrownBy(() -> parser.parse(library)) .isInstanceOf(InvalidRequestException.class) .hasMessageContaining(LIBRARY_TYPE_SYSTEM) - .hasMessageContaining("sql-query"); + .hasMessageContaining(SQL_QUERY_TYPE_CODE); } @Test @@ -193,7 +252,7 @@ void acceptsLibraryWithAdditionalTypeCodings() { .addCoding( new org.hl7.fhir.r4.model.Coding() .setSystem(LIBRARY_TYPE_SYSTEM) - .setCode("sql-query"))); + .setCode(SQL_QUERY_TYPE_CODE))); library .addContent() .setContentType("application/sql") @@ -292,13 +351,61 @@ void acceptsRelatedArtifactLabelWithUnderscoresAndDigits() { new RelatedArtifact() .setType(RelatedArtifactType.DEPENDSON) .setLabel("patients_2024") - .setResource("ViewDefinition/patient-view")); + .setResource("https://example.org/ViewDefinition/patient-view")); final ParsedSqlQuery result = parser.parse(library); assertThat(result.getViewReferences()).hasSize(1); assertThat(result.getViewReferences().get(0).getLabel()).isEqualTo("patients_2024"); } + // --------------------------------------------------------------------------- + // relatedArtifact.resource canonical-form invariant. + // --------------------------------------------------------------------------- + + @ParameterizedTest(name = "rejects non-canonical resource ''{0}''") + @ValueSource( + strings = { + "ViewDefinition/abc", + "Library/abc", + "patient-view", + "https://example.org/V#section" + }) + void rejectsNonCanonicalResourceReference(final String resource) { + final Library library = createMinimalLibrary("SELECT 1"); + library.addRelatedArtifact( + new RelatedArtifact() + .setType(RelatedArtifactType.DEPENDSON) + .setLabel("t") + .setResource(resource)); + + assertThatThrownBy(() -> parser.parse(library)) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining("relatedArtifact.resource") + .hasMessageContaining(resource) + .hasMessageContaining("canonical URL"); + } + + @ParameterizedTest(name = "accepts canonical resource ''{0}''") + @ValueSource( + strings = { + "https://example.org/ViewDefinition/patient-view", + "https://example.org/ViewDefinition/patient-view|2.0", + "http://example.org/Library/base", + "urn:uuid:53fefa32-fcbb-4ff8-8a92-55ee120877b7" + }) + void acceptsCanonicalResourceReference(final String resource) { + final Library library = createMinimalLibrary("SELECT 1"); + library.addRelatedArtifact( + new RelatedArtifact() + .setType(RelatedArtifactType.DEPENDSON) + .setLabel("t") + .setResource(resource)); + + final ParsedSqlQuery result = parser.parse(library); + assertThat(result.getViewReferences()).hasSize(1); + assertThat(result.getViewReferences().get(0).getCanonicalUrl()).isEqualTo(resource); + } + // --------------------------------------------------------------------------- // Parameter profile invariants. // --------------------------------------------------------------------------- @@ -362,7 +469,7 @@ void acceptsSqlContentTypeWithDialect() { assertThat(result.getSql()).isEqualTo("SELECT 1"); } - /** Creates a minimal Library resource with the given SQL as Base64-encoded content. */ + /** Creates a minimal SQLQuery Library resource with the given SQL as Base64-encoded content. */ private Library createMinimalLibrary(final String sql) { final Library library = new Library(); library.setStatus(PublicationStatus.ACTIVE); @@ -378,6 +485,8 @@ private Library createMinimalLibrary(final String sql) { private static CodeableConcept sqlQueryTypeCoding() { return new CodeableConcept() .addCoding( - new org.hl7.fhir.r4.model.Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode("sql-query")); + new org.hl7.fhir.r4.model.Coding() + .setSystem(LIBRARY_TYPE_SYSTEM) + .setCode(SQL_QUERY_TYPE_CODE)); } } diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryAuthTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryAuthTest.java new file mode 100644 index 0000000000..ee46fc0894 --- /dev/null +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryAuthTest.java @@ -0,0 +1,238 @@ +/* + * 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 org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatNoException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import au.csiro.pathling.config.AuthorizationConfiguration; +import au.csiro.pathling.config.ServerConfiguration; +import au.csiro.pathling.config.SqlQueryConfiguration; +import au.csiro.pathling.encoders.FhirEncoders; +import au.csiro.pathling.encoders.ViewDefinitionResource; +import au.csiro.pathling.encoders.ViewDefinitionResource.ColumnComponent; +import au.csiro.pathling.encoders.ViewDefinitionResource.SelectComponent; +import au.csiro.pathling.errors.AccessDeniedError; +import au.csiro.pathling.io.source.DataSource; +import au.csiro.pathling.read.ReadExecutor; +import au.csiro.pathling.test.SpringBootUnitTest; +import au.csiro.pathling.views.FhirView; +import ca.uhn.fhir.context.FhirContext; +import jakarta.annotation.Nonnull; +import java.util.List; +import java.util.Map; +import org.apache.spark.sql.Dataset; +import org.apache.spark.sql.Row; +import org.apache.spark.sql.SparkSession; +import org.hl7.fhir.r4.model.CodeType; +import org.hl7.fhir.r4.model.Library; +import org.hl7.fhir.r4.model.Reference; +import org.hl7.fhir.r4.model.StringType; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.security.core.authority.AuthorityUtils; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.oauth2.jwt.Jwt; +import org.springframework.security.oauth2.server.resource.authentication.JwtAuthenticationToken; + +/** + * Security tests for the {@code $sqlquery-*} resolution path, wiring the real {@link ViewResolver}, + * {@link LibraryReferenceResolver}, and {@link SqlDependencyResolver} with authorisation enabled. + * Verifies the metadata-resource authorisation matrix: a stored ViewDefinition dependency (resolved + * by canonical URL) requires {@code ViewDefinition} READ, a stored SQLView dependency requires + * {@code Library} READ, the per-projected-resource READ still applies at each leaf, and a + * request-supplied (inline) view requires no metadata READ. + * + * @author John Grimes + */ +@SpringBootUnitTest +class SqlQueryAuthTest { + + private static final String PV_URL = "https://example.org/ViewDefinition/pv"; + private static final String BASE_URL = "https://example.org/Library/base"; + + @Autowired private SparkSession spark; + @Autowired private FhirEncoders fhirEncoders; + @Autowired private FhirContext fhirContext; + + private ReadExecutor readExecutor; + private DataSource dataSource; + private LibraryReferenceResolver libraryReferenceResolver; + private SqlDependencyResolver resolver; + + @BeforeEach + void setUp() { + readExecutor = mock(ReadExecutor.class); + dataSource = mock(DataSource.class); + // Default to no matches; individual tests stub the datasets they need. + when(dataSource.read("ViewDefinition")).thenReturn(viewDefinitionDataset()); + when(dataSource.read("Library")).thenReturn(libraryDataset()); + + final ServerConfiguration serverConfiguration = new ServerConfiguration(); + final AuthorizationConfiguration auth = new AuthorizationConfiguration(); + auth.setEnabled(true); + serverConfiguration.setAuth(auth); + serverConfiguration.setSqlQuery(new SqlQueryConfiguration()); + + final ViewResolver viewResolver = + new ViewResolver(dataSource, fhirEncoders, serverConfiguration, fhirContext); + libraryReferenceResolver = + new LibraryReferenceResolver(readExecutor, dataSource, fhirEncoders, serverConfiguration); + resolver = + new SqlDependencyResolver( + viewResolver, libraryReferenceResolver, new SqlLibraryParser(), serverConfiguration); + } + + @AfterEach + void tearDown() { + SecurityContextHolder.clearContext(); + } + + @Test + void storedViewDefinitionDependencyRequiresViewDefinitionRead() { + when(dataSource.read("ViewDefinition")) + .thenReturn(viewDefinitionDataset(simpleViewDefinition("pv", PV_URL, "Patient"))); + + // Projected-resource READ alone is not enough; the ViewDefinition metadata READ is required. + setSecurityContext("pathling:read:Patient"); + assertThatThrownBy(() -> resolver.resolve(sqlQuery(PV_URL), Map.of())) + .isInstanceOf(AccessDeniedError.class) + .hasMessageContaining("ViewDefinition"); + + setSecurityContext("pathling:read:ViewDefinition", "pathling:read:Patient"); + assertThatNoException().isThrownBy(() -> resolver.resolve(sqlQuery(PV_URL), Map.of())); + } + + @Test + void projectedResourceReadStillRequiredAtTheLeaf() { + when(dataSource.read("ViewDefinition")) + .thenReturn(viewDefinitionDataset(simpleViewDefinition("pv", PV_URL, "Patient"))); + + // ViewDefinition READ without the projected Patient READ is still denied. + setSecurityContext("pathling:read:ViewDefinition"); + assertThatThrownBy(() -> resolver.resolve(sqlQuery(PV_URL), Map.of())) + .isInstanceOf(AccessDeniedError.class) + .hasMessageContaining("Patient"); + } + + @Test + void storedSqlViewDependencyRequiresLibraryRead() { + final Library base = + SqlLibraryFixtures.sqlViewWithUrl(BASE_URL, "SELECT * FROM pv", "pv", PV_URL); + base.setId("base"); + when(dataSource.read("Library")).thenReturn(libraryDataset(base)); + when(dataSource.read("ViewDefinition")) + .thenReturn(viewDefinitionDataset(simpleViewDefinition("pv", PV_URL, "Patient"))); + + // Holding the transitive ViewDefinition and projected reads, but not Library READ, is denied. + setSecurityContext("pathling:read:ViewDefinition", "pathling:read:Patient"); + assertThatThrownBy(() -> resolver.resolve(sqlQuery(BASE_URL), Map.of())) + .isInstanceOf(AccessDeniedError.class) + .hasMessageContaining("Library"); + + setSecurityContext( + "pathling:read:Library", "pathling:read:ViewDefinition", "pathling:read:Patient"); + assertThatNoException().isThrownBy(() -> resolver.resolve(sqlQuery(BASE_URL), Map.of())); + } + + @Test + void inlineSuppliedViewRequiresNoMetadataRead() { + // A request-supplied (inline) view is not read from storage, so resolving it needs no metadata + // READ - even with no authorities granted. + setSecurityContext("pathling:sqlquery-run"); + final FhirView supplied = + FhirView.ofResource("Patient") + .select(FhirView.columns(FhirView.column("id", "id"))) + .build(); + + assertThatNoException() + .isThrownBy(() -> resolver.resolve(sqlQuery(PV_URL), Map.of(PV_URL, supplied))); + } + + @Test + void topLevelQueryReferenceRequiresLibraryRead() { + final Library base = SqlLibraryFixtures.sqlView("SELECT 1"); + base.setId("base"); + when(readExecutor.read("Library", "base")).thenReturn(base); + + // The top-level by-reference resolution goes through LibraryReferenceResolver, which enforces + // the Library metadata READ. + setSecurityContext("pathling:sqlquery-run"); + assertThatThrownBy(() -> libraryReferenceResolver.resolve(new Reference("Library/base"))) + .isInstanceOf(AccessDeniedError.class) + .hasMessageContaining("Library"); + + setSecurityContext("pathling:read:Library"); + assertThat(libraryReferenceResolver.resolve(new Reference("Library/base"))).isNotNull(); + } + + // --------------------------------------------------------------------------- + // Helpers. + // --------------------------------------------------------------------------- + + @Nonnull + private Dataset viewDefinitionDataset(@Nonnull final ViewDefinitionResource... views) { + return spark + .createDataset(List.of(views), fhirEncoders.of(ViewDefinitionResource.class)) + .toDF(); + } + + @Nonnull + private Dataset libraryDataset(@Nonnull final Library... libraries) { + return spark.createDataset(List.of(libraries), fhirEncoders.of("Library")).toDF(); + } + + @Nonnull + private static ParsedSqlQuery sqlQuery(@Nonnull final String resource) { + return new ParsedSqlQuery( + "SELECT * FROM t", + List.of(new ViewArtifactReference("t", resource)), + List.of(), + SqlLibraryParser.SQL_QUERY_TYPE_CODE); + } + + private void setSecurityContext(final String... authorities) { + final Jwt jwt = Jwt.withTokenValue("mock").header("alg", "none").claim("sub", "user").build(); + final JwtAuthenticationToken auth = + new JwtAuthenticationToken(jwt, AuthorityUtils.createAuthorityList(authorities)); + SecurityContextHolder.getContext().setAuthentication(auth); + } + + @Nonnull + private static ViewDefinitionResource simpleViewDefinition( + @Nonnull final String id, @Nonnull final String url, @Nonnull final String resourceType) { + final ViewDefinitionResource view = new ViewDefinitionResource(); + view.setId(id); + view.setUrl(url); + view.setName(new StringType(id + "_view")); + view.setResource(new CodeType(resourceType)); + view.setStatus(new CodeType("active")); + final SelectComponent select = new SelectComponent(); + final ColumnComponent column = new ColumnComponent(); + column.setName(new StringType("id")); + column.setPath(new StringType("id")); + select.getColumn().add(column); + view.getSelect().add(select); + return view; + } +} diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportExecutorTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportExecutorTest.java index 6f14c70622..f5b661a8ae 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportExecutorTest.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportExecutorTest.java @@ -138,12 +138,13 @@ void outputNameFollowsPrecedenceQueryNameThenLibraryNameThenGenerated() { final PreparedSqlQuery prepared = new PreparedSqlQuery( new SqlQueryRequest( - new ParsedSqlQuery("SELECT 1", List.of(), List.of()), + new ParsedSqlQuery( + "SELECT 1", List.of(), List.of(), SqlLibraryParser.SQL_QUERY_TYPE_CODE), SqlQueryOutputFormat.NDJSON, true, null, Map.of()), - Map.of()); + new ResolvedDependencyGraph(List.of(), Map.of(), Map.of())); // query.name wins. assertThat(new QueryInput("explicit", "lib", prepared).getEffectiveName(0)) @@ -174,9 +175,13 @@ private static SqlQueryExportRequest request( private static QueryInput queryInput(final String name) { final ParsedSqlQuery parsedQuery = - new ParsedSqlQuery("SELECT id FROM patients", List.of(), List.of()); + new ParsedSqlQuery( + "SELECT id FROM patients", List.of(), List.of(), SqlLibraryParser.SQL_QUERY_TYPE_CODE); final SqlQueryRequest request = new SqlQueryRequest(parsedQuery, SqlQueryOutputFormat.NDJSON, true, null, Map.of()); - return new QueryInput(name, null, new PreparedSqlQuery(request, Map.of())); + return new QueryInput( + name, + null, + new PreparedSqlQuery(request, new ResolvedDependencyGraph(List.of(), Map.of(), Map.of()))); } } diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportFormatIT.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportFormatIT.java index 385e4e6820..636f69748d 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportFormatIT.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportFormatIT.java @@ -232,9 +232,9 @@ private Map failingLibrary() { List.of( Map.of( "system", - SqlQueryLibraryParser.LIBRARY_TYPE_SYSTEM, + SqlLibraryParser.LIBRARY_TYPE_SYSTEM, "code", - SqlQueryLibraryParser.LIBRARY_TYPE_CODE)))); + SqlLibraryParser.SQL_QUERY_TYPE_CODE)))); final String sql = "SELECT nonexistent_column FROM patients"; library.put( "content", @@ -254,7 +254,7 @@ private Map failingLibrary() { "label", "patients", "resource", - "ViewDefinition/" + SqlQueryExportTestConfiguration.PATIENT_VIEW_ID))); + SqlQueryExportTestConfiguration.PATIENT_VIEW_URL))); return library; } } diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportProviderIT.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportProviderIT.java index 6996486079..514d795735 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportProviderIT.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportProviderIT.java @@ -322,9 +322,9 @@ private Map inlineSqlLibrary() { List.of( Map.of( "system", - SqlQueryLibraryParser.LIBRARY_TYPE_SYSTEM, + SqlLibraryParser.LIBRARY_TYPE_SYSTEM, "code", - SqlQueryLibraryParser.LIBRARY_TYPE_CODE)))); + SqlLibraryParser.SQL_QUERY_TYPE_CODE)))); final String sql = "SELECT id, family_name FROM patients ORDER BY id"; library.put( "content", @@ -344,16 +344,17 @@ private Map inlineSqlLibrary() { "label", "patients", "resource", - "ViewDefinition/" + SqlQueryExportTestConfiguration.PATIENT_VIEW_ID))); + SqlQueryExportTestConfiguration.PATIENT_VIEW_URL))); return library; } - /** An inline Patient ViewDefinition whose id matches the inline query's relatedArtifact. */ + /** An inline Patient ViewDefinition whose url matches the inline query's relatedArtifact. */ @Nonnull private Map inlineViewDefinition() { final Map view = new LinkedHashMap<>(); view.put("resourceType", "ViewDefinition"); view.put("id", SqlQueryExportTestConfiguration.PATIENT_VIEW_ID); + view.put("url", SqlQueryExportTestConfiguration.PATIENT_VIEW_URL); view.put("name", "supplied_patient_view"); view.put("resource", "Patient"); view.put("status", "active"); diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportRequestParserTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportRequestParserTest.java index 3cbfea5acd..34bfda7f11 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportRequestParserTest.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportRequestParserTest.java @@ -70,11 +70,17 @@ void setUp() { deltaLake); final ParsedSqlQuery parsedQuery = - new ParsedSqlQuery("SELECT id FROM patients", java.util.List.of(), java.util.List.of()); + new ParsedSqlQuery( + "SELECT id FROM patients", + java.util.List.of(), + java.util.List.of(), + SqlLibraryParser.SQL_QUERY_TYPE_CODE); final SqlQueryRequest request = new SqlQueryRequest(parsedQuery, SqlQueryOutputFormat.NDJSON, true, null, Map.of()); when(pipeline.prepare(any(), any(), any(), any(), any(), any(), any())) - .thenReturn(new PreparedSqlQuery(request, Map.of())); + .thenReturn( + new PreparedSqlQuery( + request, new ResolvedDependencyGraph(java.util.List.of(), Map.of(), Map.of()))); when(libraryReferenceResolver.resolve(any())).thenReturn(new Library()); } diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportTestConfiguration.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportTestConfiguration.java index 06dd6c1c61..b20c1c05c2 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportTestConfiguration.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryExportTestConfiguration.java @@ -17,8 +17,8 @@ package au.csiro.pathling.operations.sqlquery; -import static au.csiro.pathling.operations.sqlquery.SqlQueryLibraryParser.LIBRARY_TYPE_CODE; -import static au.csiro.pathling.operations.sqlquery.SqlQueryLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.SQL_QUERY_TYPE_CODE; import au.csiro.pathling.encoders.FhirEncoders; import au.csiro.pathling.encoders.ViewDefinitionResource; @@ -69,9 +69,17 @@ public class SqlQueryExportTestConfiguration { /** Id of the stored Patient ViewDefinition referenced by the patient query. */ public static final String PATIENT_VIEW_ID = "patient-bp"; + /** Canonical URL of the stored Patient ViewDefinition (final segment differs from its id). */ + public static final String PATIENT_VIEW_URL = + "https://pathling.csiro.au/test/ViewDefinition/PatientBp"; + /** Id of the stored Observation ViewDefinition referenced by the observation query. */ public static final String OBSERVATION_VIEW_ID = "observation-weight"; + /** Canonical URL of the stored Observation ViewDefinition. */ + public static final String OBSERVATION_VIEW_URL = + "https://pathling.csiro.au/test/ViewDefinition/ObservationWeight"; + /** Id of the stored SQLQuery Library that selects patients. */ public static final String PATIENT_QUERY_ID = "patient-bp-query"; @@ -103,14 +111,14 @@ public QueryableDataSource deltaLake( "patient_bp_query", "SELECT id, family_name FROM patients ORDER BY id", "patients", - "ViewDefinition/" + PATIENT_VIEW_ID)); + PATIENT_VIEW_URL)); resources.add( sqlLibrary( OBSERVATION_QUERY_ID, "observation_weight_query", "SELECT id, subject, weight_kg FROM observations ORDER BY id", "observations", - "ViewDefinition/" + OBSERVATION_VIEW_ID)); + OBSERVATION_VIEW_URL)); resources.add(parameterisedPatientQuery()); // p1 has an older meta.lastUpdated than p2/p3, so a `_since` filter can scope it out. resources.add(patient("p1", "Smith", P1_LAST_UPDATED)); @@ -128,6 +136,7 @@ public QueryableDataSource deltaLake( private static ViewDefinitionResource patientView() { final ViewDefinitionResource view = new ViewDefinitionResource(); view.setId(PATIENT_VIEW_ID); + view.setUrl(PATIENT_VIEW_URL); view.setName(new StringType("patient_view")); view.setResource(new CodeType("Patient")); view.setStatus(new CodeType("active")); @@ -142,6 +151,7 @@ private static ViewDefinitionResource patientView() { private static ViewDefinitionResource observationView() { final ViewDefinitionResource view = new ViewDefinitionResource(); view.setId(OBSERVATION_VIEW_ID); + view.setUrl(OBSERVATION_VIEW_URL); view.setName(new StringType("observation_view")); view.setResource(new CodeType("Observation")); view.setStatus(new CodeType("active")); @@ -162,7 +172,7 @@ private static Library parameterisedPatientQuery() { "patient_by_family_query", "SELECT id, family_name FROM patients WHERE family_name = :familyName", "patients", - "ViewDefinition/" + PATIENT_VIEW_ID); + PATIENT_VIEW_URL); library.addParameter( new ParameterDefinition().setName("familyName").setUse(ParameterUse.IN).setType("string")); return library; @@ -182,7 +192,7 @@ static Library sqlLibrary( library.setStatus(PublicationStatus.ACTIVE); library.setType( new CodeableConcept() - .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(LIBRARY_TYPE_CODE))); + .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(SQL_QUERY_TYPE_CODE))); final Attachment content = new Attachment(); content.setContentType("application/sql"); content.setData(sql.getBytes(StandardCharsets.UTF_8)); diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryPipelineTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryPipelineTest.java index 320f95dcb2..90401804ed 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryPipelineTest.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryPipelineTest.java @@ -29,7 +29,6 @@ import au.csiro.pathling.views.FhirView; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import org.apache.spark.sql.Dataset; @@ -39,73 +38,77 @@ import org.junit.jupiter.api.Test; /** - * Unit tests for {@link SqlQueryPipeline}, verifying that it orchestrates parsing, view resolution, - * static validation, and execution across its collaborators. The collaborators are mocked, so the - * test exercises the pipeline's wiring rather than the Spark-backed execution itself. + * Unit tests for {@link SqlQueryPipeline}, verifying that it orchestrates parsing, dependency + * resolution, static validation, and execution across its collaborators. The collaborators are + * mocked, so the test exercises the pipeline's wiring rather than the Spark-backed execution + * itself. * * @author John Grimes */ class SqlQueryPipelineTest { private SqlQueryRequestParser requestParser; - private ViewResolver viewResolver; + private SqlDependencyResolver dependencyResolver; private SqlQueryExecutor executor; - private SqlValidator sqlValidator; private SqlQueryPipeline pipeline; private Library library; private SqlQueryRequest request; - private Map resolvedViews; + private ResolvedDependencyGraph graph; @BeforeEach void setUp() { requestParser = mock(SqlQueryRequestParser.class); - viewResolver = mock(ViewResolver.class); + dependencyResolver = mock(SqlDependencyResolver.class); executor = mock(SqlQueryExecutor.class); - sqlValidator = mock(SqlValidator.class); - pipeline = new SqlQueryPipeline(requestParser, viewResolver, executor, sqlValidator); + pipeline = new SqlQueryPipeline(requestParser, dependencyResolver, executor); library = new Library(); final ParsedSqlQuery parsedQuery = new ParsedSqlQuery( "SELECT id FROM patients", List.of(new ViewArtifactReference("patients", "ViewDefinition/patient-view")), - List.of()); + List.of(), + SqlLibraryParser.SQL_QUERY_TYPE_CODE); request = new SqlQueryRequest(parsedQuery, SqlQueryOutputFormat.NDJSON, true, null, Map.of()); - final FhirView view = mock(FhirView.class); - resolvedViews = Map.of("patients", view); + final ResolvedViewDefinition leaf = + new ResolvedViewDefinition("ViewDefinition/patient-view", mock(FhirView.class)); + graph = + new ResolvedDependencyGraph( + List.of(leaf), + Map.of("patients", "ViewDefinition/patient-view"), + Map.of("ViewDefinition/patient-view", leaf)); } @Test - void prepareParsesAndResolvesViewsWithSuppliedViews() { + void prepareParsesAndResolvesDependencyGraphWithSuppliedViews() { final FhirView suppliedView = mock(FhirView.class); final Map supplied = Map.of("patient-view", suppliedView); when(requestParser.parse(eq(library), eq("ndjson"), any(), any(), any(), any())) .thenReturn(request); - when(viewResolver.resolve(request.getParsedQuery().getViewReferences(), supplied)) - .thenReturn(resolvedViews); + when(dependencyResolver.resolve(request.getParsedQuery(), supplied)).thenReturn(graph); final PreparedSqlQuery prepared = pipeline.prepare(library, "ndjson", null, null, null, null, supplied); assertThat(prepared.getRequest()).isSameAs(request); - assertThat(prepared.getResolvedViews()).isEqualTo(resolvedViews); + assertThat(prepared.getDependencyGraph()).isSameAs(graph); verify(requestParser).parse(eq(library), eq("ndjson"), any(), any(), any(), any()); - verify(viewResolver).resolve(request.getParsedQuery().getViewReferences(), supplied); + verify(dependencyResolver).resolve(request.getParsedQuery(), supplied); } @Test - void validateStaticallyValidatesSqlWithDeclaredLabels() { - final PreparedSqlQuery prepared = new PreparedSqlQuery(request, resolvedViews); + void validateStaticallyDelegatesToExecutor() { + final PreparedSqlQuery prepared = new PreparedSqlQuery(request, graph); pipeline.validateStatically(prepared); - verify(sqlValidator).validate("SELECT id FROM patients", Set.of("patients")); + verify(executor).validateStatically(request, graph); } @Test void executeDelegatesToExecutorAndPassesDatasetToConsumer() { - final PreparedSqlQuery prepared = new PreparedSqlQuery(request, resolvedViews); + final PreparedSqlQuery prepared = new PreparedSqlQuery(request, graph); final DataSource dataSource = mock(DataSource.class); @SuppressWarnings("unchecked") final Dataset dataset = mock(Dataset.class); @@ -118,12 +121,12 @@ void executeDelegatesToExecutorAndPassesDatasetToConsumer() { return null; }) .when(executor) - .execute(eq(request), eq(resolvedViews), eq(dataSource), eq("req-1"), any()); + .execute(eq(request), eq(graph), eq(dataSource), eq("req-1"), any()); final AtomicReference> received = new AtomicReference<>(); pipeline.execute(prepared, dataSource, "req-1", received::set); assertThat(received.get()).isSameAs(dataset); - verify(executor).execute(eq(request), eq(resolvedViews), eq(dataSource), eq("req-1"), any()); + verify(executor).execute(eq(request), eq(graph), eq(dataSource), eq("req-1"), any()); } } diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRequestParserTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRequestParserTest.java index a318fb546c..6ff0c9659d 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRequestParserTest.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRequestParserTest.java @@ -57,7 +57,7 @@ class SqlQueryRequestParserTest { @BeforeEach void setUp() { - parser = new SqlQueryRequestParser(new SqlQueryLibraryParser()); + parser = new SqlQueryRequestParser(new SqlLibraryParser()); } // --------------------------------------------------------------------------- @@ -303,6 +303,50 @@ void fallsBackToNdjsonWhenAcceptHeaderDoesNotMatch() { assertThat(request.getOutputFormat()).isEqualTo(SqlQueryOutputFormat.NDJSON); } + // --------------------------------------------------------------------------- + // Top-level SQLView (US3). + // --------------------------------------------------------------------------- + + @Test + void acceptsTopLevelSqlViewLibrary() { + // A SQLView supplied as the top-level resource parses as a parameter-less query. + final Library library = SqlLibraryFixtures.sqlView("SELECT 1"); + + final SqlQueryRequest request = parser.parse(library, null, null, null, null, null); + + assertThat(request.getParsedQuery().isView()).isTrue(); + assertThat(request.getParameterBindings()).isEmpty(); + } + + @Test + void rejectsParametersSuppliedWithTopLevelSqlView() { + // A SQLView declares no parameter, so any supplied binding must be rejected. + final Library library = SqlLibraryFixtures.sqlView("SELECT 1"); + final Parameters params = new Parameters(); + params.addParameter().setName("min_age").setValue(new IntegerType(42)); + + assertThatThrownBy(() -> parser.parse(library, null, null, null, null, params)) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining("min_age"); + } + + @Test + void rejectsTopLevelLibraryWithUnknownType() { + // A Library whose type is neither sql-query nor sql-view is rejected. + final Library library = new Library(); + library.setStatus(PublicationStatus.ACTIVE); + library.setType( + new CodeableConcept() + .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode("logic-library"))); + library + .addContent() + .setContentType("application/sql") + .setData("SELECT 1".getBytes(StandardCharsets.UTF_8)); + + assertThatThrownBy(() -> parser.parse(library, null, null, null, null, null)) + .isInstanceOf(InvalidRequestException.class); + } + /** Builds a minimal SQLQuery-typed Library carrying the given SQL. */ private static Library libraryWithSql(final String sql) { final Library library = new Library(); diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunDeltaIT.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunDeltaIT.java index d926e01b50..ec6a516b0a 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunDeltaIT.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunDeltaIT.java @@ -17,8 +17,8 @@ package au.csiro.pathling.operations.sqlquery; -import static au.csiro.pathling.operations.sqlquery.SqlQueryLibraryParser.LIBRARY_TYPE_CODE; -import static au.csiro.pathling.operations.sqlquery.SqlQueryLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.SQL_QUERY_TYPE_CODE; import static org.assertj.core.api.Assertions.assertThat; import au.csiro.pathling.util.TestDataSetup; @@ -128,10 +128,9 @@ void cleanup() throws IOException { */ @Test void runsSqlAgainstDeltaBackedViewDefinition() { - final String viewId = createPatientViewDefinition(); + final String viewUrl = createPatientViewDefinition(); final Library library = - sqlQueryLibrary( - "SELECT id FROM patients ORDER BY id LIMIT 5", "patients", "ViewDefinition/" + viewId); + sqlQueryLibrary("SELECT id FROM patients ORDER BY id LIMIT 5", "patients", viewUrl); final String body = postOk( @@ -149,10 +148,16 @@ void runsSqlAgainstDeltaBackedViewDefinition() { } } + /** + * Creates a Patient ViewDefinition carrying a canonical url whose final segment differs from the + * server-assigned logical id, then returns that url so the dependency can be referenced by it. + */ @Nonnull private String createPatientViewDefinition() { + final String viewUrl = "https://pathling.csiro.au/test/ViewDefinition/DeltaPatients"; final Map view = new LinkedHashMap<>(); view.put("resourceType", "ViewDefinition"); + view.put("url", viewUrl); view.put("name", "patients"); view.put("resource", "Patient"); view.put("status", "active"); @@ -163,21 +168,15 @@ private String createPatientViewDefinition() { select.put("column", List.of(column)); view.put("select", List.of(select)); - final EntityExchangeResult result = - webTestClient - .post() - .uri("http://localhost:" + port + "/fhir/ViewDefinition") - .header("Content-Type", "application/fhir+json") - .bodyValue(GSON.toJson(view)) - .exchange() - .expectStatus() - .isCreated() - .expectBody() - .returnResult(); - final String location = - Objects.requireNonNull(result.getResponseHeaders().getFirst("Location")); - final String[] parts = location.split("/"); - return parts[parts.length - 1]; + webTestClient + .post() + .uri("http://localhost:" + port + "/fhir/ViewDefinition") + .header("Content-Type", "application/fhir+json") + .bodyValue(GSON.toJson(view)) + .exchange() + .expectStatus() + .isCreated(); + return viewUrl; } @Nonnull @@ -211,7 +210,7 @@ private Library sqlQueryLibrary( library.setStatus(PublicationStatus.ACTIVE); library.setType( new CodeableConcept() - .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(LIBRARY_TYPE_CODE))); + .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(SQL_QUERY_TYPE_CODE))); final Attachment content = new Attachment(); content.setContentType("application/sql"); content.setData(sql.getBytes(StandardCharsets.UTF_8)); diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunProviderIT.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunProviderIT.java index db61f3d0fb..4d5037bf5f 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunProviderIT.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunProviderIT.java @@ -17,8 +17,8 @@ package au.csiro.pathling.operations.sqlquery; -import static au.csiro.pathling.operations.sqlquery.SqlQueryLibraryParser.LIBRARY_TYPE_CODE; -import static au.csiro.pathling.operations.sqlquery.SqlQueryLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.SQL_QUERY_TYPE_CODE; import static org.assertj.core.api.Assertions.assertThat; import ca.uhn.fhir.context.FhirContext; @@ -456,7 +456,7 @@ private Library sqlQueryLibrary(@Nonnull final String sql) { library.setStatus(PublicationStatus.ACTIVE); library.setType( new CodeableConcept() - .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(LIBRARY_TYPE_CODE))); + .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(SQL_QUERY_TYPE_CODE))); final Attachment content = new Attachment(); content.setContentType("application/sql"); content.setData(sql.getBytes(StandardCharsets.UTF_8)); diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunSecurityIT.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunSecurityIT.java index 4e6d738872..e2b54c0e40 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunSecurityIT.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunSecurityIT.java @@ -17,8 +17,8 @@ package au.csiro.pathling.operations.sqlquery; -import static au.csiro.pathling.operations.sqlquery.SqlQueryLibraryParser.LIBRARY_TYPE_CODE; -import static au.csiro.pathling.operations.sqlquery.SqlQueryLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.SQL_QUERY_TYPE_CODE; import static org.assertj.core.api.Assertions.assertThat; import ca.uhn.fhir.context.FhirContext; @@ -185,7 +185,7 @@ private Library sqlQueryLibrary(@Nonnull final String sql) { library.setStatus(PublicationStatus.ACTIVE); library.setType( new CodeableConcept() - .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(LIBRARY_TYPE_CODE))); + .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(SQL_QUERY_TYPE_CODE))); final Attachment content = new Attachment(); content.setContentType("application/sql"); content.setData(sql.getBytes(StandardCharsets.UTF_8)); diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunWithViewDefinitionsIT.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunWithViewDefinitionsIT.java index 9d1d60b7eb..4beb512d65 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunWithViewDefinitionsIT.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryRunWithViewDefinitionsIT.java @@ -17,8 +17,8 @@ package au.csiro.pathling.operations.sqlquery; -import static au.csiro.pathling.operations.sqlquery.SqlQueryLibraryParser.LIBRARY_TYPE_CODE; -import static au.csiro.pathling.operations.sqlquery.SqlQueryLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.SQL_QUERY_TYPE_CODE; import static org.assertj.core.api.Assertions.assertThat; import ca.uhn.fhir.context.FhirContext; @@ -79,10 +79,10 @@ class SqlQueryRunWithViewDefinitionsIT { private static final Gson GSON = new Gson(); private static final String VIEW_REFERENCE = - "ViewDefinition/" + SqlQueryViewDefinitionTestConfiguration.PATIENT_VIEW_ID; + SqlQueryViewDefinitionTestConfiguration.PATIENT_VIEW_URL; private static final String OBSERVATION_VIEW_REFERENCE = - "ViewDefinition/" + SqlQueryViewDefinitionTestConfiguration.OBSERVATION_VIEW_ID; + SqlQueryViewDefinitionTestConfiguration.OBSERVATION_VIEW_URL; @LocalServerPort int port; @@ -186,9 +186,12 @@ void runsSqlAgainstViewWithPathlingUdfInFhirPath() { } @Test - void returns400WhenReferencedViewDefinitionDoesNotExist() { + void returnsErrorWhenReferencedViewDefinitionDoesNotExist() { final Library library = - sqlQueryLibrary("SELECT id FROM patients", "patients", "ViewDefinition/does-not-exist"); + sqlQueryLibrary( + "SELECT id FROM patients", + "patients", + "https://pathling.csiro.au/test/ViewDefinition/does-not-exist"); webTestClient .post() @@ -232,7 +235,7 @@ private Library sqlQueryLibrary( library.setStatus(PublicationStatus.ACTIVE); library.setType( new CodeableConcept() - .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(LIBRARY_TYPE_CODE))); + .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(SQL_QUERY_TYPE_CODE))); final Attachment content = new Attachment(); content.setContentType("application/sql"); content.setData(sql.getBytes(StandardCharsets.UTF_8)); diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryViewDefinitionTestConfiguration.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryViewDefinitionTestConfiguration.java index 1a774a486f..56366c3074 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryViewDefinitionTestConfiguration.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlQueryViewDefinitionTestConfiguration.java @@ -65,6 +65,14 @@ public class SqlQueryViewDefinitionTestConfiguration { /** The id of the pre-loaded Patient ViewDefinition referenced by tests. */ public static final String PATIENT_VIEW_ID = "patient-view"; + /** + * The canonical URL of the pre-loaded Patient ViewDefinition. The URL's final segment ({@code + * Patients}) deliberately differs from the logical id ({@link #PATIENT_VIEW_ID}), so resolving a + * dependency by this URL exercises the case the id-based resolution could not handle. + */ + public static final String PATIENT_VIEW_URL = + "https://pathling.csiro.au/test/ViewDefinition/Patients"; + /** * The id of the pre-loaded Observation ViewDefinition referenced by tests. Its FHIRPath uses * {@code .toString()} on a Decimal, which compiles to the Pathling-registered {@code @@ -72,6 +80,10 @@ public class SqlQueryViewDefinitionTestConfiguration { */ public static final String OBSERVATION_VIEW_ID = "observation-view"; + /** The canonical URL of the pre-loaded Observation ViewDefinition. */ + public static final String OBSERVATION_VIEW_URL = + "https://pathling.csiro.au/test/ViewDefinition/Observations"; + @Primary @Bean @Nonnull @@ -95,6 +107,7 @@ public QueryableDataSource deltaLake( private static ViewDefinitionResource patientView() { final ViewDefinitionResource view = new ViewDefinitionResource(); view.setId(PATIENT_VIEW_ID); + view.setUrl(PATIENT_VIEW_URL); view.setName(new StringType("patient_view")); view.setResource(new CodeType("Patient")); view.setStatus(new CodeType("active")); @@ -115,6 +128,7 @@ private static ViewDefinitionResource patientView() { private static ViewDefinitionResource observationView() { final ViewDefinitionResource view = new ViewDefinitionResource(); view.setId(OBSERVATION_VIEW_ID); + view.setUrl(OBSERVATION_VIEW_URL); view.setName(new StringType("observation_view")); view.setResource(new CodeType("Observation")); view.setStatus(new CodeType("active")); diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlValidatorTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlValidatorTest.java index 5b4c5eb9da..ba45f9e771 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlValidatorTest.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlValidatorTest.java @@ -183,6 +183,38 @@ void acceptsSelectWithCoalesce() { .doesNotThrowAnyException(); } + // ------------------------------------------------------------------------- + // String concatenation. The || operator parses directly to a Concat + // expression (AstBuilder maps CONCAT_PIPE to Concat), so it never appears + // as an UnresolvedFunction and must be present in the expression allow-list + // in its own right. The equivalent concat() function call resolves to an + // UnresolvedFunction named 'concat', which is already permitted because it + // is a built-in, so the two spellings must behave consistently. + // ------------------------------------------------------------------------- + + @Test + void acceptsStringConcatenationOperator() { + assertThatCode(() -> validate("SELECT a || b FROM t", "t")).doesNotThrowAnyException(); + } + + @Test + void acceptsConcatFunction() { + assertThatCode(() -> validate("SELECT concat(a, b) FROM t", "t")).doesNotThrowAnyException(); + } + + @Test + void acceptsDrugStrengthStyleConcatenation() { + // Mirrors the real-world "Drug strength" view that triggered this fix: a + // human-readable strength label assembled from several columns using ||. + assertThatCode( + () -> + validate( + "SELECT numerator_value || ' ' || numerator_unit || '/' || denominator_value" + + " || ' ' || denominator_unit AS strength FROM t", + "t")) + .doesNotThrowAnyException(); + } + // ------------------------------------------------------------------------- // Invalid SQL — should reject DDL and DML. // ------------------------------------------------------------------------- @@ -356,6 +388,79 @@ void acceptsRelationInLabelSet() { assertThatCode(() -> validate("SELECT * FROM patients", "patients")).doesNotThrowAnyException(); } + // ------------------------------------------------------------------------- + // Relation references inside CTE definition bodies. A WITH node exposes its + // CTE bodies as innerChildren rather than children, so the strict walk must + // descend into them explicitly. Without this, an undeclared table referenced + // inside a CTE body (for example an OMOP 'concept' vocabulary table) slips + // past static validation and surfaces as an opaque 500 from Spark's analyser + // instead of a clean rejection. + // ------------------------------------------------------------------------- + + @Test + void rejectsUndeclaredTableInsideCteBody() { + // The relation is named only inside the CTE definition, never at the top level. + assertThatThrownBy(() -> validate("WITH cr AS (SELECT * FROM concept) SELECT * FROM cr")) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining("undeclared table") + .hasMessageContaining("concept"); + } + + @Test + void rejectsUndeclaredTableInJoinInsideCteBody() { + // Mirrors the real OMOP concept-resolution CTE, which joins several undeclared + // vocabulary tables within its body. + assertThatThrownBy( + () -> + validate( + "WITH cr AS (" + + " SELECT * FROM concept c" + + " LEFT JOIN concept_relationship rel ON c.concept_id = rel.concept_id_1" + + ") SELECT * FROM cr")) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining("undeclared table"); + } + + @Test + void rejectsUndeclaredLabelInsideCteBody() { + // A label reference inside a CTE body must still be checked against the declared + // set: when 'patients' is not declared, the relation is undeclared and rejected. + assertThatThrownBy(() -> validate("WITH foo AS (SELECT * FROM patients) SELECT * FROM foo")) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining("undeclared table") + .hasMessageContaining("patients"); + } + + @Test + void rejectsDatasourceShortNameFileReadInsideCteBody() { + // The datasource short-name file-read lane must stay closed inside CTE bodies too. + assertThatThrownBy( + () -> validate("WITH leak AS (SELECT * FROM csv.`/etc/passwd`) SELECT * FROM leak")) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining("undeclared table"); + } + + @Test + void rejectsUndeclaredTableInNestedCteBody() { + // Nested WITH: the inner CTE 'b' is defined inside the body of the outer CTE 'a'. + assertThatThrownBy( + () -> + validate( + "WITH a AS (WITH b AS (SELECT * FROM concept) SELECT * FROM b) SELECT * FROM" + + " a")) + .isInstanceOf(InvalidRequestException.class) + .hasMessageContaining("undeclared table") + .hasMessageContaining("concept"); + } + + @Test + void acceptsCteBodyReferencingDeclaredLabel() { + // The positive counterpart: once the body is walked, a declared label inside it + // resolves cleanly and the query is accepted. + assertThatCode(() -> validate("WITH cr AS (SELECT * FROM concept) SELECT * FROM cr", "concept")) + .doesNotThrowAnyException(); + } + // ------------------------------------------------------------------------- // View-derived Pathling UDFs flow through unaffected. ViewDefinition // FHIRPath expressions compile to Spark plans that contain ScalaUDFs (for diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlViewExportProviderIT.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlViewExportProviderIT.java new file mode 100644 index 0000000000..e43315d3e0 --- /dev/null +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlViewExportProviderIT.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.operations.sqlquery; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.nio.file.Path; +import java.util.Map; +import lombok.extern.slf4j.Slf4j; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.ResourceAccessMode; +import org.junit.jupiter.api.parallel.ResourceLock; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.context.annotation.Import; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.context.DynamicPropertyRegistry; +import org.springframework.test.context.DynamicPropertySource; + +/** + * End-to-end integration test for {@code $sqlquery-export} against stored SQLView {@code Library} + * resources, confirming that the asynchronous export resolves and materialises the same dependency + * graph as {@code $sqlquery-run} (US3 - run or export a SQLView directly). + * + *

    Backed by {@link SqlViewTestConfiguration}, which holds the ViewDefinitions, SQLViews, and + * FHIR data the queries resolve against. + * + * @author John Grimes + */ +@Slf4j +@Tag("IntegrationTest") +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@ResourceLock(value = "wiremock", mode = ResourceAccessMode.READ_WRITE) +@ActiveProfiles({"integration-test"}) +@Import(SqlViewTestConfiguration.class) +class SqlViewExportProviderIT extends AbstractSqlQueryExportIT { + + @DynamicPropertySource + static void configureProperties(final DynamicPropertyRegistry registry) { + final Path warehouseDir = + Path.of("src/test/resources/test-data/bulk/fhir/delta").toAbsolutePath(); + registry.add("pathling.storage.warehouseUrl", () -> "file://" + warehouseDir); + } + + @Test + void instanceLevelExportOfStoredSqlViewProducesComposedRows() throws InterruptedException { + // Exporting a stored SQLView at the instance level resolves its ViewDefinition dependency and + // writes the composed rows, identically to running it. + final Map manifest = + exportToCompletion( + instanceLevelUri(SqlViewTestConfiguration.ACTIVE_PATIENTS_ID), emptyParameters()); + + assertThat(findParamValue(manifest, "status", "valueCode")).isEqualTo("completed"); + assertThat(paramsByName(manifest, "output")).hasSize(1); + final String content = + download(partValue(paramsByName(manifest, "output").get(0), "location", "valueUri")); + assertThat(content) + .contains("\"family_name\":\"Smith\"") + .contains("\"family_name\":\"Johnson\"") + .contains("\"family_name\":\"Williams\""); + } + + @Test + void typeLevelExportWithQueryReferenceToSqlViewProducesOutput() throws InterruptedException { + // A SQLView referenced as a top-level query at the type level exports identically. + final Map manifest = + exportToCompletion( + typeLevelUri(), storedQuery(SqlViewTestConfiguration.ACTIVE_PATIENTS_ID, null)); + + assertThat(findParamValue(manifest, "status", "valueCode")).isEqualTo("completed"); + assertThat(paramsByName(manifest, "output")).hasSize(1); + final String content = + download(partValue(paramsByName(manifest, "output").get(0), "location", "valueUri")); + assertThat(content).contains("\"family_name\":\"Smith\""); + } +} diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlViewRunProviderIT.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlViewRunProviderIT.java new file mode 100644 index 0000000000..2c2d3c48ec --- /dev/null +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlViewRunProviderIT.java @@ -0,0 +1,351 @@ +/* + * 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.operations.sqlquery.SqlLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.SQL_QUERY_TYPE_CODE; +import static org.assertj.core.api.Assertions.assertThat; + +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.parser.IParser; +import com.google.gson.Gson; +import jakarta.annotation.Nonnull; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import lombok.extern.slf4j.Slf4j; +import org.hl7.fhir.r4.model.Attachment; +import org.hl7.fhir.r4.model.CodeableConcept; +import org.hl7.fhir.r4.model.Coding; +import org.hl7.fhir.r4.model.Enumerations.PublicationStatus; +import org.hl7.fhir.r4.model.Library; +import org.hl7.fhir.r4.model.RelatedArtifact; +import org.hl7.fhir.r4.model.RelatedArtifact.RelatedArtifactType; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.ResourceAccessMode; +import org.junit.jupiter.api.parallel.ResourceLock; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.web.server.LocalServerPort; +import org.springframework.context.annotation.Import; +import org.springframework.http.MediaType; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.context.DynamicPropertyRegistry; +import org.springframework.test.context.DynamicPropertySource; +import org.springframework.test.web.reactive.server.EntityExchangeResult; +import org.springframework.test.web.reactive.server.WebTestClient; + +/** + * End-to-end integration test for the {@code $sqlquery-run} operation against stored SQLView {@code + * Library} resources. Drives the full pipeline - dependency resolution, graph materialisation, and + * result streaming - for a SQLQuery that composes a SQLView (US1) and for nested and cyclic graphs + * (US2). + * + *

    Backed by {@link SqlViewTestConfiguration}, which substitutes an in-memory data source holding + * the ViewDefinitions, SQLViews, and FHIR data the queries resolve against. + * + * @author John Grimes + */ +@Slf4j +@Tag("IntegrationTest") +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@ResourceLock(value = "wiremock", mode = ResourceAccessMode.READ_WRITE) +@ActiveProfiles({"integration-test"}) +@Import(SqlViewTestConfiguration.class) +class SqlViewRunProviderIT { + + private static final Gson GSON = new Gson(); + + @LocalServerPort int port; + + @Autowired WebTestClient webTestClient; + + @Autowired private FhirContext fhirContext; + + private IParser jsonParser; + + @DynamicPropertySource + static void configureProperties(final DynamicPropertyRegistry registry) { + final Path warehouseDir = + Path.of("src/test/resources/test-data/bulk/fhir/delta").toAbsolutePath(); + registry.add("pathling.storage.warehouseUrl", () -> "file://" + warehouseDir); + } + + @BeforeEach + void setup() { + webTestClient = + webTestClient + .mutate() + .codecs(configurer -> configurer.defaultCodecs().maxInMemorySize(100 * 1024 * 1024)) + // Nested SQLView graphs plan a deeper Spark query, so allow more than the 5s default. + .responseTimeout(java.time.Duration.ofSeconds(60)) + .build(); + jsonParser = fhirContext.newJsonParser(); + } + + @Test + void runsSqlQueryComposingAStoredSqlView() { + // A SQLQuery that selects from a SQLView (which itself selects from a ViewDefinition) returns + // the composed rows. + final Library library = + sqlQueryLibrary( + "SELECT id, family_name FROM ap ORDER BY id", + "ap", + SqlViewTestConfiguration.libraryUrl(SqlViewTestConfiguration.ACTIVE_PATIENTS_ID)); + + final String body = postOk(parametersJson(library)); + + final String[] lines = body.trim().split("\n"); + assertThat(lines).hasSize(3); + assertThat(body) + .contains("\"id\":\"p1\"") + .contains("\"family_name\":\"Smith\"") + .contains("\"id\":\"p2\"") + .contains("\"family_name\":\"Johnson\"") + .contains("\"id\":\"p3\"") + .contains("\"family_name\":\"Williams\""); + } + + @Test + void runsSqlQueryComposingANestedSqlViewChain() { + // SQLQuery -> refined-patients (SQLView) -> active-patients (SQLView) -> patient-view (VD). + // refined-patients filters out Johnson, so two rows remain. + final Library library = + sqlQueryLibrary( + "SELECT id, family_name FROM rp ORDER BY id", + "rp", + SqlViewTestConfiguration.libraryUrl(SqlViewTestConfiguration.REFINED_PATIENTS_ID)); + + final String body = postOk(parametersJson(library)); + + final String[] lines = body.trim().split("\n"); + assertThat(lines).hasSize(2); + assertThat(body).contains("Smith").contains("Williams").doesNotContain("Johnson"); + } + + @Test + void runsSqlQueryOverADiamondOfSqlViews() { + // left and right both depend on the shared SQLView; the join returns one row per patient, + // confirming both arms observe the same shared materialisation. + final Library library = new Library(); + library.setStatus(PublicationStatus.ACTIVE); + library.setType( + new CodeableConcept() + .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(SQL_QUERY_TYPE_CODE))); + final Attachment content = new Attachment(); + content.setContentType("application/sql"); + content.setData( + "SELECT l.id, l.family_name FROM l JOIN r ON l.id = r.id ORDER BY l.id" + .getBytes(StandardCharsets.UTF_8)); + library.addContent(content); + library.addRelatedArtifact( + new RelatedArtifact() + .setType(RelatedArtifactType.DEPENDSON) + .setLabel("l") + .setResource( + SqlViewTestConfiguration.libraryUrl(SqlViewTestConfiguration.LEFT_PATIENTS_ID))); + library.addRelatedArtifact( + new RelatedArtifact() + .setType(RelatedArtifactType.DEPENDSON) + .setLabel("r") + .setResource( + SqlViewTestConfiguration.libraryUrl(SqlViewTestConfiguration.RIGHT_PATIENTS_ID))); + + final String body = postOk(parametersJson(library)); + + final String[] lines = body.trim().split("\n"); + assertThat(lines).hasSize(3); + assertThat(body).contains("Smith").contains("Johnson").contains("Williams"); + } + + @Test + void runsStoredSqlViewAtInstanceLevel() { + // A stored SQLView supplied as the top-level resource of the instance-level operation executes + // as a parameter-less query and returns its rows. + final String body = + getOk( + "/fhir/Library/" + + SqlViewTestConfiguration.ACTIVE_PATIENTS_ID + + "/$sqlquery-run?_format=ndjson"); + + final String[] lines = body.trim().split("\n"); + assertThat(lines).hasSize(3); + assertThat(body).contains("Smith").contains("Johnson").contains("Williams"); + } + + @Test + void runsSqlViewByQueryReferenceAtSystemLevel() { + // A SQLView supplied as a top-level queryReference at the system level executes and returns its + // rows. + final String body = + postOk( + queryReferenceParametersJson("Library/" + SqlViewTestConfiguration.ACTIVE_PATIENTS_ID)); + + final String[] lines = body.trim().split("\n"); + assertThat(lines).hasSize(3); + assertThat(body).contains("Smith").contains("Johnson").contains("Williams"); + } + + @Test + void returnsErrorWhenReferencedSqlViewDoesNotExist() { + final Library library = + sqlQueryLibrary( + "SELECT id FROM missing", + "missing", + SqlViewTestConfiguration.libraryUrl("does-not-exist")); + + // The error names the failing label and reference so the client can act on it. + final String body = postExpect4xx(parametersJson(library)); + assertThat(body).contains("missing").contains("does-not-exist"); + } + + @Test + void rejectsCyclicSqlViewGraphWith400() { + // cycle-a -> cycle-b -> cycle-a must be rejected before any SQL executes. + final Library library = + sqlQueryLibrary( + "SELECT * FROM a", + "a", + SqlViewTestConfiguration.libraryUrl(SqlViewTestConfiguration.CYCLE_A_ID)); + + final String body = postExpect4xx(parametersJson(library)); + assertThat(body).containsIgnoringCase("cycl"); + } + + @Nonnull + private String postExpect4xx(@Nonnull final String body) { + final EntityExchangeResult result = + webTestClient + .post() + .uri("http://localhost:" + port + "/fhir/$sqlquery-run") + .header("Content-Type", "application/fhir+json") + .header("Accept", SqlQueryOutputFormat.NDJSON.getContentType()) + .bodyValue(body) + .exchange() + .expectStatus() + .is4xxClientError() + .expectBody() + .returnResult(); + final byte[] payload = result.getResponseBodyContent(); + return payload == null ? "" : new String(payload, StandardCharsets.UTF_8); + } + + @Nonnull + private String getOk(@Nonnull final String path) { + final EntityExchangeResult result = + webTestClient + .get() + .uri("http://localhost:" + port + path) + .header("Accept", SqlQueryOutputFormat.NDJSON.getContentType()) + .exchange() + .expectStatus() + .isOk() + .expectBody() + .returnResult(); + return new String( + Objects.requireNonNull(result.getResponseBodyContent()), StandardCharsets.UTF_8); + } + + @Nonnull + private String queryReferenceParametersJson(@Nonnull final String reference) { + final Map parameters = new LinkedHashMap<>(); + parameters.put("resourceType", "Parameters"); + final List> parameterList = new ArrayList<>(); + + final Map queryReferenceParam = new LinkedHashMap<>(); + queryReferenceParam.put("name", "queryReference"); + queryReferenceParam.put("valueReference", Map.of("reference", reference)); + parameterList.add(queryReferenceParam); + + final Map formatParam = new LinkedHashMap<>(); + formatParam.put("name", "_format"); + formatParam.put("valueString", SqlQueryOutputFormat.NDJSON.getCode()); + parameterList.add(formatParam); + + parameters.put("parameter", parameterList); + return GSON.toJson(parameters); + } + + @Nonnull + private String postOk(@Nonnull final String body) { + final EntityExchangeResult result = + webTestClient + .post() + .uri("http://localhost:" + port + "/fhir/$sqlquery-run") + .header("Content-Type", "application/fhir+json") + .header("Accept", SqlQueryOutputFormat.NDJSON.getContentType()) + .bodyValue(body) + .exchange() + .expectStatus() + .isOk() + .expectHeader() + .contentTypeCompatibleWith( + MediaType.parseMediaType(SqlQueryOutputFormat.NDJSON.getContentType())) + .expectBody() + .returnResult(); + return new String( + Objects.requireNonNull(result.getResponseBodyContent()), StandardCharsets.UTF_8); + } + + @Nonnull + private Library sqlQueryLibrary( + @Nonnull final String sql, @Nonnull final String label, @Nonnull final String resource) { + final Library library = new Library(); + library.setStatus(PublicationStatus.ACTIVE); + library.setType( + new CodeableConcept() + .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(SQL_QUERY_TYPE_CODE))); + final Attachment content = new Attachment(); + content.setContentType("application/sql"); + content.setData(sql.getBytes(StandardCharsets.UTF_8)); + library.addContent(content); + library.addRelatedArtifact( + new RelatedArtifact() + .setType(RelatedArtifactType.DEPENDSON) + .setLabel(label) + .setResource(resource)); + return library; + } + + @Nonnull + private String parametersJson(@Nonnull final Library library) { + final String libraryJson = jsonParser.encodeResourceToString(library); + final Map parameters = new LinkedHashMap<>(); + parameters.put("resourceType", "Parameters"); + final List> parameterList = new ArrayList<>(); + + final Map queryResourceParam = new LinkedHashMap<>(); + queryResourceParam.put("name", "queryResource"); + queryResourceParam.put("resource", GSON.fromJson(libraryJson, Map.class)); + parameterList.add(queryResourceParam); + + final Map formatParam = new LinkedHashMap<>(); + formatParam.put("name", "_format"); + formatParam.put("valueString", SqlQueryOutputFormat.NDJSON.getCode()); + parameterList.add(formatParam); + + parameters.put("parameter", parameterList); + return GSON.toJson(parameters); + } +} diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlViewTestConfiguration.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlViewTestConfiguration.java new file mode 100644 index 0000000000..a655f1c453 --- /dev/null +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/SqlViewTestConfiguration.java @@ -0,0 +1,222 @@ +/* + * 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.operations.sqlquery.SqlLibraryParser.LIBRARY_TYPE_SYSTEM; +import static au.csiro.pathling.operations.sqlquery.SqlLibraryParser.SQL_VIEW_TYPE_CODE; + +import au.csiro.pathling.encoders.FhirEncoders; +import au.csiro.pathling.encoders.ViewDefinitionResource; +import au.csiro.pathling.encoders.ViewDefinitionResource.ColumnComponent; +import au.csiro.pathling.encoders.ViewDefinitionResource.SelectComponent; +import au.csiro.pathling.library.PathlingContext; +import au.csiro.pathling.library.io.source.QueryableDataSource; +import au.csiro.pathling.util.CustomObjectDataSource; +import jakarta.annotation.Nonnull; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import org.apache.spark.sql.SparkSession; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.r4.model.Attachment; +import org.hl7.fhir.r4.model.CodeType; +import org.hl7.fhir.r4.model.CodeableConcept; +import org.hl7.fhir.r4.model.Coding; +import org.hl7.fhir.r4.model.Enumerations.PublicationStatus; +import org.hl7.fhir.r4.model.Library; +import org.hl7.fhir.r4.model.Patient; +import org.hl7.fhir.r4.model.RelatedArtifact; +import org.hl7.fhir.r4.model.RelatedArtifact.RelatedArtifactType; +import org.hl7.fhir.r4.model.StringType; +import org.springframework.boot.test.context.TestConfiguration; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Primary; + +/** + * Test configuration that overrides the production {@code deltaLake} data source with an in-memory + * one pre-loaded with FHIR resources, ViewDefinitions, and SQLView {@code Library} resources. Used + * by the SQLView end-to-end ITs to drive queries that resolve a SQLView dependency, a nested chain, + * a diamond, and a cycle, against real FHIR data. + * + *

    The stored graph is: + * + *

      + *
    • {@code ViewDefinition/patient-view} - Patient projection (id, family_name). + *
    • {@code Library/active-patients} - SQLView over {@code patient-view}. + *
    • {@code Library/refined-patients} - SQLView over {@code active-patients} (a nested chain). + *
    • {@code Library/cycle-a} / {@code Library/cycle-b} - a mutually-referencing cycle. + *
    + * + * @author John Grimes + */ +@TestConfiguration +public class SqlViewTestConfiguration { + + /** Base canonical URL for test ViewDefinitions. */ + private static final String VIEW_DEFINITION_BASE = + "https://pathling.csiro.au/test/ViewDefinition/"; + + /** Base canonical URL for test SQLView Libraries. */ + private static final String LIBRARY_BASE = "https://pathling.csiro.au/test/Library/"; + + /** The id of the pre-loaded Patient ViewDefinition. */ + public static final String PATIENT_VIEW_ID = "patient-view"; + + /** + * The canonical URL of the pre-loaded Patient ViewDefinition. The URL's final segment ({@code + * Patients}) differs from the logical id, so dependencies that resolve it by URL exercise the + * case the id-based resolution could not handle. + */ + public static final String PATIENT_VIEW_URL = VIEW_DEFINITION_BASE + "Patients"; + + /** + * Returns the canonical URL of a stored SQLView Library given its local id. + * + * @param id the SQLView's logical id + * @return the SQLView's canonical url + */ + @Nonnull + public static String libraryUrl(@Nonnull final String id) { + return LIBRARY_BASE + id; + } + + /** The id of the SQLView over the Patient ViewDefinition. */ + public static final String ACTIVE_PATIENTS_ID = "active-patients"; + + /** The id of the SQLView over {@link #ACTIVE_PATIENTS_ID} (a nested chain). */ + public static final String REFINED_PATIENTS_ID = "refined-patients"; + + /** The id of one half of a mutually-referencing cycle. */ + public static final String CYCLE_A_ID = "cycle-a"; + + /** The id of the other half of a mutually-referencing cycle. */ + public static final String CYCLE_B_ID = "cycle-b"; + + /** The id of the SQLView shared by both arms of a diamond. */ + public static final String SHARED_PATIENTS_ID = "shared-patients"; + + /** The id of the left arm of a diamond, over {@link #SHARED_PATIENTS_ID}. */ + public static final String LEFT_PATIENTS_ID = "left-patients"; + + /** The id of the right arm of a diamond, over {@link #SHARED_PATIENTS_ID}. */ + public static final String RIGHT_PATIENTS_ID = "right-patients"; + + @Primary + @Bean + @Nonnull + public QueryableDataSource deltaLake( + @Nonnull final SparkSession sparkSession, + @Nonnull final PathlingContext pathlingContext, + @Nonnull final FhirEncoders fhirEncoders) { + final List resources = new ArrayList<>(); + resources.add(patientView()); + resources.add( + sqlView( + ACTIVE_PATIENTS_ID, + "SELECT id, family_name FROM patient_view", + Map.of("patient_view", PATIENT_VIEW_URL))); + resources.add( + sqlView( + REFINED_PATIENTS_ID, + "SELECT id, family_name FROM ap WHERE family_name <> 'Johnson'", + Map.of("ap", libraryUrl(ACTIVE_PATIENTS_ID)))); + resources.add(sqlView(CYCLE_A_ID, "SELECT * FROM b", Map.of("b", libraryUrl(CYCLE_B_ID)))); + resources.add(sqlView(CYCLE_B_ID, "SELECT * FROM a", Map.of("a", libraryUrl(CYCLE_A_ID)))); + resources.add( + sqlView( + SHARED_PATIENTS_ID, + "SELECT id, family_name FROM patient_view", + Map.of("patient_view", PATIENT_VIEW_URL))); + resources.add( + sqlView( + LEFT_PATIENTS_ID, + "SELECT id, family_name FROM sp", + Map.of("sp", libraryUrl(SHARED_PATIENTS_ID)))); + resources.add( + sqlView( + RIGHT_PATIENTS_ID, "SELECT id FROM sp", Map.of("sp", libraryUrl(SHARED_PATIENTS_ID)))); + resources.add(patient("p1", "Smith")); + resources.add(patient("p2", "Johnson")); + resources.add(patient("p3", "Williams")); + return new CustomObjectDataSource(sparkSession, pathlingContext, fhirEncoders, resources); + } + + @Nonnull + private static ViewDefinitionResource patientView() { + final ViewDefinitionResource view = new ViewDefinitionResource(); + view.setId(PATIENT_VIEW_ID); + view.setUrl(PATIENT_VIEW_URL); + view.setName(new StringType("patient_view")); + view.setResource(new CodeType("Patient")); + view.setStatus(new CodeType("active")); + final SelectComponent select = new SelectComponent(); + select.getColumn().add(column("id", "id")); + select.getColumn().add(column("family_name", "name.first().family")); + view.getSelect().add(select); + return view; + } + + /** + * Builds a stored SQLView Library with the given id, SQL, and depends-on dependencies (label to + * resource reference, iteration order preserved). + */ + @Nonnull + private static Library sqlView( + @Nonnull final String id, + @Nonnull final String sql, + @Nonnull final Map dependenciesByLabel) { + final Library library = new Library(); + library.setId(id); + library.setUrl(libraryUrl(id)); + library.setStatus(PublicationStatus.ACTIVE); + library.setType( + new CodeableConcept() + .addCoding(new Coding().setSystem(LIBRARY_TYPE_SYSTEM).setCode(SQL_VIEW_TYPE_CODE))); + final Attachment content = new Attachment(); + content.setContentType("application/sql"); + content.setData(sql.getBytes(StandardCharsets.UTF_8)); + library.addContent(content); + new LinkedHashMap<>(dependenciesByLabel) + .forEach( + (label, resource) -> + library.addRelatedArtifact( + new RelatedArtifact() + .setType(RelatedArtifactType.DEPENDSON) + .setLabel(label) + .setResource(resource))); + return library; + } + + @Nonnull + private static ColumnComponent column(@Nonnull final String name, @Nonnull final String path) { + final ColumnComponent column = new ColumnComponent(); + column.setName(new StringType(name)); + column.setPath(new StringType(path)); + return column; + } + + @Nonnull + private static Patient patient(@Nonnull final String id, @Nonnull final String family) { + final Patient patient = new Patient(); + patient.setId(id); + patient.addName().setFamily(family); + return patient; + } +} diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/ViewRegistrationServiceTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/ViewRegistrationServiceTest.java index c42e3985ba..13c1212e6e 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/ViewRegistrationServiceTest.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/ViewRegistrationServiceTest.java @@ -95,6 +95,74 @@ void resolveTempViewNameFallsBackToHashWhenSanitisedRequestIdIsEmpty() { assertThat(dashes).isNotEqualTo(slashes); } + @Test + void resolveTempViewNameDerivesFromCanonicalKeyNotLabel() { + // The temp view name is keyed by the resolved resource's canonical key, so a key carrying a + // slash and dash (ViewDefinition/patient-view) is sanitised into a legal Spark identifier. + final String name = + ViewRegistrationService.resolveTempViewName("req1", "ViewDefinition/patient-view"); + assertThat(name).startsWith("sqlquery_req1_").doesNotContain("/").doesNotContain("-"); + } + + @Test + void resolveTempViewNameDerivesFromCanonicalUrlKey() { + // The canonical key is now a full canonical URL (optionally url|version); the scheme, slashes, + // dots, and version pipe must all sanitise into a legal Spark identifier. + final String name = + ViewRegistrationService.resolveTempViewName( + "req1", "https://example.org/ViewDefinition/Patients|2"); + assertThat(name) + .startsWith("sqlquery_req1_") + .doesNotContain("/") + .doesNotContain(":") + .doesNotContain(".") + .doesNotContain("|"); + } + + @Test + void resolveTempViewNameGivesDistinctNamesToDistinctCanonicalUrlKeys() { + // A bare-url key and a url|version key must not collapse to the same temp view name. + final String bare = + ViewRegistrationService.resolveTempViewName("req1", "https://example.org/V"); + final String versioned = + ViewRegistrationService.resolveTempViewName("req1", "https://example.org/V|2"); + assertThat(bare).isNotEqualTo(versioned); + } + + @Test + void resolveTempViewNameGivesDistinctNamesToDistinctKeys() { + // Two nodes that happen to share a label but resolve to different resources are keyed by their + // distinct canonical keys, so their temp views never collide. + final String left = ViewRegistrationService.resolveTempViewName("req1", "ViewDefinition/a"); + final String right = ViewRegistrationService.resolveTempViewName("req1", "ViewDefinition/b"); + assertThat(left).isNotEqualTo(right); + } + + // --------------------------------------------------------------------------- + // SQLView materialisation. + // --------------------------------------------------------------------------- + + @Test + void buildSqlViewRewritesAgainstChildTempViewsBeforeRunning() { + // Materialise a child node under its canonical key, then build a SQLView whose SQL selects from + // a label that maps to that child. The SQL must be rewritten to the child's temp view name + // before running, so the SQLView observes the child's rows. + final Dataset childData = singleColumnDataset("value", List.of("x", "y")); + final String childKey = "ViewDefinition/child"; + final String childViewName = service.registerDataset(childKey, childData, "req1"); + try { + final ResolvedSqlView node = + new ResolvedSqlView("Library/parent", "SELECT value FROM t", Map.of("t", childKey)); + + final Dataset result = service.buildSqlView(node, Map.of(childKey, childViewName)); + + assertThat(result.collectAsList().stream().map(row -> row.getString(0)).toList()) + .containsExactlyInAnyOrder("x", "y"); + } finally { + service.dropViews(List.of(childViewName)); + } + } + // --------------------------------------------------------------------------- // SQL rewriting. // --------------------------------------------------------------------------- diff --git a/server/src/test/java/au/csiro/pathling/operations/sqlquery/ViewResolverTest.java b/server/src/test/java/au/csiro/pathling/operations/sqlquery/ViewResolverTest.java index bde2660025..492ab17c68 100644 --- a/server/src/test/java/au/csiro/pathling/operations/sqlquery/ViewResolverTest.java +++ b/server/src/test/java/au/csiro/pathling/operations/sqlquery/ViewResolverTest.java @@ -18,120 +18,145 @@ package au.csiro.pathling.operations.sqlquery; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import au.csiro.pathling.config.AuthorizationConfiguration; import au.csiro.pathling.config.ServerConfiguration; +import au.csiro.pathling.encoders.FhirEncoders; import au.csiro.pathling.encoders.ViewDefinitionResource; import au.csiro.pathling.encoders.ViewDefinitionResource.ColumnComponent; import au.csiro.pathling.encoders.ViewDefinitionResource.SelectComponent; -import au.csiro.pathling.errors.ResourceNotFoundError; -import au.csiro.pathling.read.ReadExecutor; -import au.csiro.pathling.views.FhirView; +import au.csiro.pathling.io.source.DataSource; +import au.csiro.pathling.test.SpringBootUnitTest; import ca.uhn.fhir.context.FhirContext; -import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; import java.util.List; -import java.util.Map; +import java.util.Optional; +import org.apache.spark.sql.Dataset; +import org.apache.spark.sql.Row; +import org.apache.spark.sql.SparkSession; import org.hl7.fhir.r4.model.CodeType; import org.hl7.fhir.r4.model.StringType; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; /** - * Unit tests for {@link ViewResolver} covering id extraction, label-order preservation, and the - * read-error and parse-error wrapping paths. + * Unit tests for {@link ViewResolver}, verifying that a {@code ViewDefinition} dependency resolves + * by matching its canonical {@code url} (and {@code url|version}) - never its logical id - and that + * the resolved canonical key is the resource's url plus its version. + * + * @author John Grimes */ +@SpringBootUnitTest class ViewResolverTest { - private ReadExecutor readExecutor; - private ServerConfiguration serverConfiguration; + private static final String PATIENTS_URL = "https://example.org/Patients"; + + @Autowired private SparkSession spark; + @Autowired private FhirEncoders fhirEncoders; + @Autowired private FhirContext fhirContext; + + private DataSource dataSource; private ViewResolver resolver; @BeforeEach void setUp() { - readExecutor = mock(ReadExecutor.class); - serverConfiguration = new ServerConfiguration(); + dataSource = mock(DataSource.class); + final ServerConfiguration serverConfiguration = new ServerConfiguration(); final AuthorizationConfiguration auth = new AuthorizationConfiguration(); auth.setEnabled(false); serverConfiguration.setAuth(auth); - resolver = new ViewResolver(readExecutor, serverConfiguration, FhirContext.forR4()); - } - - @Test - void resolvesEmptyReferenceListToEmptyMap() { - final Map resolved = resolver.resolve(List.of()); - assertThat(resolved).isEmpty(); + resolver = new ViewResolver(dataSource, fhirEncoders, serverConfiguration, fhirContext); } @Test - void resolvesSingleReferenceByBareId() { - when(readExecutor.read("ViewDefinition", "patient-view")) - .thenReturn(simpleViewDefinition("patient-view", "Patient")); + void resolvesAViewDefinitionByUrlNotLogicalId() { + // The logical id ("vd-abc") deliberately differs from the URL's final segment ("Patients"). + when(dataSource.read("ViewDefinition")) + .thenReturn(dataset(viewDefinition("vd-abc", PATIENTS_URL, null, "active", "Patient"))); - final Map resolved = - resolver.resolve(List.of(new ViewArtifactReference("patients", "patient-view"))); + final Optional resolved = + resolver.resolveStoredViewDefinition(new ViewArtifactReference("patients", PATIENTS_URL)); - assertThat(resolved).containsOnlyKeys("patients"); - assertThat(resolved.get("patients").getResource()).isEqualTo("Patient"); + assertThat(resolved).isPresent(); + assertThat(resolved.get().getCanonicalKey()).isEqualTo(PATIENTS_URL); + assertThat(resolved.get().getView().getResource()).isEqualTo("Patient"); } @Test - void extractsIdFromCanonicalUrl() { - when(readExecutor.read("ViewDefinition", "obs-view")) - .thenReturn(simpleViewDefinition("obs-view", "Observation")); + void returnsEmptyWhenNoStoredViewDefinitionMatchesTheUrl() { + when(dataSource.read("ViewDefinition")) + .thenReturn(dataset(viewDefinition("vd-abc", PATIENTS_URL, null, "active", "Patient"))); - final Map resolved = - resolver.resolve( - List.of( - new ViewArtifactReference("obs", "https://example.org/ViewDefinition/obs-view"))); + final Optional resolved = + resolver.resolveStoredViewDefinition( + new ViewArtifactReference("missing", "https://example.org/Missing")); - assertThat(resolved).containsOnlyKeys("obs"); - assertThat(resolved.get("obs").getResource()).isEqualTo("Observation"); + assertThat(resolved).isEmpty(); } @Test - void preservesLabelOrderAcrossMultipleReferences() { - when(readExecutor.read("ViewDefinition", "a")).thenReturn(simpleViewDefinition("a", "Patient")); - when(readExecutor.read("ViewDefinition", "b")) - .thenReturn(simpleViewDefinition("b", "Observation")); - when(readExecutor.read("ViewDefinition", "c")) - .thenReturn(simpleViewDefinition("c", "Condition")); - - final Map resolved = - resolver.resolve( - List.of( - new ViewArtifactReference("first", "a"), - new ViewArtifactReference("second", "b"), - new ViewArtifactReference("third", "c"))); - - assertThat(resolved.keySet()).containsExactly("first", "second", "third"); + void selectsTheExactVersionWhenOneIsRequested() { + when(dataSource.read("ViewDefinition")) + .thenReturn( + dataset( + viewDefinition("v1", PATIENTS_URL, "1", "active", "Patient"), + viewDefinition("v2", PATIENTS_URL, "2", "active", "Patient"))); + + final Optional resolved = + resolver.resolveStoredViewDefinition( + new ViewArtifactReference("patients", PATIENTS_URL + "|2")); + + assertThat(resolved).isPresent(); + assertThat(resolved.get().getCanonicalKey()).isEqualTo(PATIENTS_URL + "|2"); } @Test - void wrapsReadExecutorFailureWithLabelAndReference() { - when(readExecutor.read("ViewDefinition", "missing")) - .thenThrow(new ResourceNotFoundError("not there")); + void selectsTheLatestActiveVersionForABareUrl() { + when(dataSource.read("ViewDefinition")) + .thenReturn( + dataset( + viewDefinition("v1", PATIENTS_URL, "1", "retired", "Patient"), + viewDefinition("v2", PATIENTS_URL, "2", "active", "Patient"))); + + final Optional resolved = + resolver.resolveStoredViewDefinition(new ViewArtifactReference("patients", PATIENTS_URL)); + + // The resolved canonical key is the chosen resource's url plus its version. + assertThat(resolved).isPresent(); + assertThat(resolved.get().getCanonicalKey()).isEqualTo(PATIENTS_URL + "|2"); + } - final List refs = - List.of(new ViewArtifactReference("patients", "missing")); + // --------------------------------------------------------------------------- + // Helpers. + // --------------------------------------------------------------------------- - assertThatThrownBy(() -> resolver.resolve(refs)) - .isInstanceOf(InvalidRequestException.class) - .hasMessageContaining("patients") - .hasMessageContaining("missing"); + @Nonnull + private Dataset dataset(@Nonnull final ViewDefinitionResource... views) { + return spark + .createDataset(List.of(views), fhirEncoders.of(ViewDefinitionResource.class)) + .toDF(); } @Nonnull - private static ViewDefinitionResource simpleViewDefinition( - @Nonnull final String id, @Nonnull final String resourceType) { + private static ViewDefinitionResource viewDefinition( + @Nonnull final String id, + @Nonnull final String url, + @Nullable final String version, + @Nonnull final String status, + @Nonnull final String resourceType) { final ViewDefinitionResource view = new ViewDefinitionResource(); view.setId(id); + view.setUrl(url); + if (version != null) { + view.setVersion(version); + } view.setName(new StringType(id + "_view")); view.setResource(new CodeType(resourceType)); - view.setStatus(new CodeType("active")); + view.setStatus(new CodeType(status)); final SelectComponent select = new SelectComponent(); final ColumnComponent column = new ColumnComponent(); column.setName(new StringType("id")); diff --git a/server/src/test/java/au/csiro/pathling/operations/view/ViewExecutionHelperAuthTest.java b/server/src/test/java/au/csiro/pathling/operations/view/ViewExecutionHelperAuthTest.java new file mode 100644 index 0000000000..7730eae8a4 --- /dev/null +++ b/server/src/test/java/au/csiro/pathling/operations/view/ViewExecutionHelperAuthTest.java @@ -0,0 +1,141 @@ +/* + * 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.view; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import au.csiro.pathling.config.AuthorizationConfiguration; +import au.csiro.pathling.config.ServerConfiguration; +import au.csiro.pathling.encoders.FhirEncoders; +import au.csiro.pathling.encoders.ViewDefinitionResource; +import au.csiro.pathling.encoders.ViewDefinitionResource.ColumnComponent; +import au.csiro.pathling.encoders.ViewDefinitionResource.SelectComponent; +import au.csiro.pathling.errors.AccessDeniedError; +import au.csiro.pathling.library.io.source.QueryableDataSource; +import au.csiro.pathling.operations.compartment.GroupMemberService; +import au.csiro.pathling.operations.compartment.PatientCompartmentService; +import au.csiro.pathling.read.ReadExecutor; +import ca.uhn.fhir.context.FhirContext; +import jakarta.annotation.Nonnull; +import org.apache.spark.sql.SparkSession; +import org.hl7.fhir.r4.model.CodeType; +import org.hl7.fhir.r4.model.Reference; +import org.hl7.fhir.r4.model.StringType; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.springframework.security.core.authority.AuthorityUtils; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.oauth2.jwt.Jwt; +import org.springframework.security.oauth2.server.resource.authentication.JwtAuthenticationToken; + +/** + * Security tests for {@link ViewExecutionHelper}, verifying that resolving a stored {@code + * ViewDefinition} from a {@code viewReference} requires the {@code ViewDefinition} metadata READ + * authority, while an inline {@code viewResource} requires no such check. + * + * @author John Grimes + */ +@Tag("UnitTest") +class ViewExecutionHelperAuthTest { + + private ReadExecutor readExecutor; + private AuthorizationConfiguration authConfig; + private ViewExecutionHelper helper; + + @BeforeEach + void setUp() { + readExecutor = mock(ReadExecutor.class); + final ServerConfiguration serverConfiguration = new ServerConfiguration(); + authConfig = new AuthorizationConfiguration(); + authConfig.setEnabled(true); + serverConfiguration.setAuth(authConfig); + helper = + new ViewExecutionHelper( + mock(SparkSession.class), + mock(QueryableDataSource.class), + FhirContext.forR4Cached(), + mock(FhirEncoders.class), + mock(PatientCompartmentService.class), + mock(GroupMemberService.class), + serverConfiguration, + readExecutor); + } + + @AfterEach + void tearDown() { + SecurityContextHolder.clearContext(); + } + + @Test + void viewReferenceRequiresViewDefinitionReadAuthority() { + setSecurityContext("pathling:read:Patient"); + when(readExecutor.read("ViewDefinition", "stored")) + .thenReturn(simpleViewDefinition("stored", "Patient")); + + assertThatThrownBy(() -> helper.resolveViewInput(null, new Reference("ViewDefinition/stored"))) + .isInstanceOf(AccessDeniedError.class) + .hasMessageContaining("ViewDefinition"); + } + + @Test + void viewReferenceSucceedsWithViewDefinitionReadAuthority() { + setSecurityContext("pathling:read:ViewDefinition"); + when(readExecutor.read("ViewDefinition", "stored")) + .thenReturn(simpleViewDefinition("stored", "Patient")); + + assertThat(helper.resolveViewInput(null, new Reference("ViewDefinition/stored"))).isNotNull(); + } + + @Test + void inlineViewResourceRequiresNoViewDefinitionRead() { + // An inline viewResource is not read from storage, so the metadata READ check does not apply. + setSecurityContext("pathling:search"); + final ViewDefinitionResource inline = simpleViewDefinition("inline", "Patient"); + + assertThat(helper.resolveViewInput(inline, null)).isSameAs(inline); + } + + private void setSecurityContext(final String... authorities) { + final Jwt jwt = Jwt.withTokenValue("mock").header("alg", "none").claim("sub", "user").build(); + final JwtAuthenticationToken auth = + new JwtAuthenticationToken(jwt, AuthorityUtils.createAuthorityList(authorities)); + SecurityContextHolder.getContext().setAuthentication(auth); + } + + @Nonnull + private static ViewDefinitionResource simpleViewDefinition( + @Nonnull final String id, @Nonnull final String resourceType) { + final ViewDefinitionResource view = new ViewDefinitionResource(); + view.setId(id); + view.setName(new StringType(id + "_view")); + view.setResource(new CodeType(resourceType)); + view.setStatus(new CodeType("active")); + final SelectComponent select = new SelectComponent(); + final ColumnComponent column = new ColumnComponent(); + column.setName(new StringType("id")); + column.setPath(new StringType("id")); + select.getColumn().add(column); + view.getSelect().add(select); + return view; + } +} diff --git a/site/docs/server/authorization.md b/site/docs/server/authorization.md index a237c5dae0..85be6f9001 100644 --- a/site/docs/server/authorization.md +++ b/site/docs/server/authorization.md @@ -90,6 +90,22 @@ the resource type specified in the ViewDefinition's `resource` element. For example, a ViewDefinition targeting `Patient` resources requires `pathling:read:Patient` authority in addition to the operation authority. +### Reading view definitions and queries from storage + +Resolving a metadata resource from server storage requires `read` authority on +that resource type, in addition to the per-projected-resource checks above: + +- Resolving a `ViewDefinition` from storage requires `pathling:read:ViewDefinition`. + This applies to a `view-run` or `view-export` `viewReference`, and to a + `ViewDefinition` referenced as a dependency of a SQL query. +- Resolving a `SQLView` (a `Library`) from storage requires `pathling:read:Library`. + This applies to a `$sqlquery-run` or `$sqlquery-export` `queryReference` or + instance-level Library, and to a `SQLView` referenced as a dependency. + +A resource supplied inline in the request body (an inline `viewResource` or +`queryResource`) is not read from storage and is therefore not subject to these +metadata read checks, though the per-projected-resource read checks still apply. + ## SMART configuration When authorisation is enabled, Pathling exposes a diff --git a/site/docs/server/configuration.md b/site/docs/server/configuration.md index 393ef97110..76322d3594 100644 --- a/site/docs/server/configuration.md +++ b/site/docs/server/configuration.md @@ -183,6 +183,12 @@ limits are always applied; they cannot be disabled per request. 4xx response; a timeout that fires mid-stream aborts the connection and is recorded as a server warning. Long-running queries should use the asynchronous path. +- `pathling.sqlQuery.maxDependencyDepth` - (default: `10`) The maximum nesting + depth of the SQLView dependency graph resolved for a single query. The + top-level query's direct dependencies sit at depth one; each further level of + nested SQLView dependency increments the depth. A graph nested deeper is + rejected with a `400` before any Spark work, guarding against accidental + fan-out and runaway resolution. ### Encoding diff --git a/site/docs/server/operations/sql-export.md b/site/docs/server/operations/sql-export.md index a5a84a836f..ac5d185950 100644 --- a/site/docs/server/operations/sql-export.md +++ b/site/docs/server/operations/sql-export.md @@ -61,15 +61,27 @@ Each `query` part must supply exactly one of `query.queryReference` or `404 Not Found`. A SQLQuery `Library` declares its table sources as `relatedArtifact` entries, -each labelling a ViewDefinition the SQL references. The optional `view` -parameter supplies those ViewDefinitions at request time, matched to the -`relatedArtifact` entries by ViewDefinition id. A view the SQL references but no -`view` part supplies is read from server storage, exactly as the synchronous -operation does. Each `view` part must supply exactly one of `view.viewReference` -or `view.viewResource`; supplying both, or neither, returns `400 Bad Request`. A +each labelling a ViewDefinition or SQLView the SQL references by its canonical +URL. The optional `view` parameter supplies those ViewDefinitions at request +time, matched to the `relatedArtifact` entries by canonical URL; a supplied view +is therefore preferred over storage when its `url` matches, and a supplied view +that carries no `url` is rejected with `400 Bad Request` because it could never +satisfy a canonical reference. A view the SQL references but no `view` part +supplies is read from server storage, exactly as the synchronous operation does. +Each `view` part must supply exactly one of `view.viewReference` or +`view.viewResource`; supplying both, or neither, returns `400 Bad Request`. A supplied ViewDefinition that is well-formed but semantically invalid returns `422 Unprocessable Entity`. +A `relatedArtifact` dependency may reference a SQLView as well as a +ViewDefinition, and a SQLView may itself depend on further ViewDefinitions and +SQLViews. A SQLView may also be the top-level resource of a `query` (or the +bound Library at the instance level), running as a parameter-less query. The +dependency-graph resolution, reference disambiguation, cycle and depth limits, +and metadata-resource authorisation are identical to the synchronous operation; +see [Composing SQLViews](./sql-run.md#composing-sqlviews). These structural +rejections are returned synchronously at kick-off. + The `source` parameter (an external data source) is not supported by this server; supplying it returns `400 Bad Request`. All of these rejections are returned synchronously at kick-off. diff --git a/site/docs/server/operations/sql-run.md b/site/docs/server/operations/sql-run.md index 0711ce334b..f149cef0a3 100644 --- a/site/docs/server/operations/sql-run.md +++ b/site/docs/server/operations/sql-run.md @@ -66,15 +66,19 @@ The `$sqlquery-run` operation expects a Library that conforms to the SQLQuery profile. The relevant elements are: - `type` - must include the coding `sql-query` from - `https://sql-on-fhir.org/ig/CodeSystem/LibraryTypesCodes`. + `https://sql-on-fhir.org/ig/CodeSystem/LibraryTypesCodes`. A + [SQLView](https://build.fhir.org/ig/FHIR/sql-on-fhir-v2/StructureDefinition-SQLView.html) + (`sql-view`) is also accepted as a top-level resource and runs as a + parameter-less query. - `content` - exactly one entry with `contentType` of `application/sql` and the SQL text Base64-encoded in `data`. -- `relatedArtifact` - one entry per ViewDefinition the query references. The - `label` becomes the table name available to the SQL, and `resource` points to - the ViewDefinition (relative literal or canonical reference). +- `relatedArtifact` - one entry per dependency the query references. The `label` + becomes the table name available to the SQL, and `resource` is the canonical + URL of a ViewDefinition or a SQLView, matched against that resource's `url`. + See [Composing SQLViews](#composing-sqlviews). - `parameter` - optional declarations of named runtime parameters. Each entry with `use` of `in` must have a `name` and `type`, and the type must be a - primitive FHIR type. + primitive FHIR type. A SQLView declares no parameters. Example Library: @@ -100,7 +104,7 @@ Example Library: { "type": "depends-on", "label": "patients", - "resource": "ViewDefinition/patient-demographics" + "resource": "https://example.org/ViewDefinition/patient-demographics" } ] } @@ -148,7 +152,7 @@ Accept: application/x-ndjson { "type": "depends-on", "label": "patients", - "resource": "ViewDefinition/patient-demographics" + "resource": "https://example.org/ViewDefinition/patient-demographics" } ] } @@ -273,6 +277,60 @@ time via the `parameters` input: Each binding's name must match a declaration on the Library, and its `value[x]` must match the declared FHIR type. +## Composing SQLViews + +A `relatedArtifact` dependency may reference a +[SQLView](https://build.fhir.org/ig/FHIR/sql-on-fhir-v2/StructureDefinition-SQLView.html) +as well as a ViewDefinition. A SQLView is a reusable, named SQL query +(a `Library` with `type` of `sql-view` and no parameters) that other queries +build on as a virtual table. A SQLView may itself depend on ViewDefinitions and +other SQLViews, forming a directed acyclic graph of virtual tables that the +server resolves and executes. Each node's result is available to its referrer +under the `label` of the `relatedArtifact` that points at it; labels are scoped +to the node that declares them, so the same label may name different sources in +different nodes. + +A SQLView may also be supplied directly as the top-level `queryResource`, +`queryReference`, or instance-level Library; it then executes as a +parameter-less query and returns its rows. + +### Reference resolution + +Each `relatedArtifact.resource` is an absolute **canonical URL** of a +ViewDefinition or SQLView, resolved by matching the referenced resource's `url` +element - never its logical id: + +- The reference must be an absolute canonical URL (scheme `http://`, `https://` + or `urn:`), optionally suffixed with a single `|version`. A relative literal + form such as `ViewDefinition/abc`, a bare id, or a value carrying a fragment + is rejected with a `400` at parse time. +- `[url]|[version]` selects the resource whose `url` and `version` match + exactly; a bare `[url]` selects the latest active match (preferring `active` + status, then the greatest version string). +- ViewDefinitions are searched first, then SQLView Libraries. A canonical that + matches both a ViewDefinition and a SQLView is rejected as ambiguous (`400`); + one that matches neither is a not-found error (`404`). Both messages name the + failing label and reference. + +A ViewDefinition or SQLView must therefore carry a `url` to be referenceable as +a dependency. ViewDefinitions ingested before URL retention was added must be +re-ingested so their `url` is stored. + +A `Library` that is a `sql-query` rather than a `sql-view`, a cycle (for example +`A -> B -> A`), and a graph nested deeper than +`pathling.sqlQuery.maxDependencyDepth` are each rejected with a `400` before any +SQL executes, with a message identifying the cause. De-duplication, cycle +detection, and depth enforcement are keyed on resolved canonical identity, so a +bare-url and a `url|version` reference to the same stored resource materialise +once. + +When authorisation is enabled, resolving a ViewDefinition from storage requires +READ on `ViewDefinition`, and resolving a SQLView from storage requires READ on +`Library`, in addition to the READ check on each projected resource type. A +resource supplied inline in the request body is not read from storage and is not +subject to the metadata check. See the +[authorisation documentation](../authorization.md). + ## Resource limits Two server-configured limits are always applied to a `$sqlquery-run` @@ -283,6 +341,9 @@ invocation, regardless of any caller-supplied parameters: - `pathling.sqlQuery.timeoutSeconds` (default `60`) - the maximum wall-clock time, in seconds, that a query may run before its Spark job group is cancelled. +- `pathling.sqlQuery.maxDependencyDepth` (default `10`) - the maximum nesting + depth of the SQLView dependency graph. A graph nested deeper is rejected with + a `400` before any Spark work. Long-running queries should use the asynchronous bulk submit path rather than the synchronous `$sqlquery-run` endpoint. See the @@ -395,8 +456,8 @@ def main(): library = build_sql_query_library( sql, { - "patients": "ViewDefinition/patient-demographics", - "conditions": "ViewDefinition/conditions", + "patients": "https://example.org/ViewDefinition/patient-demographics", + "conditions": "https://example.org/ViewDefinition/conditions", }, ) diff --git a/ui/e2e/fixtures/fhirData.ts b/ui/e2e/fixtures/fhirData.ts index 938e977dfd..710f8f98d8 100644 --- a/ui/e2e/fixtures/fhirData.ts +++ b/ui/e2e/fixtures/fhirData.ts @@ -269,6 +269,7 @@ export const mockExportManifest: Parameters = { export const mockViewDefinition1 = { resourceType: "ViewDefinition", id: "patient-demographics", + url: "https://pathling.example/ViewDefinition/PatientDemographics", name: "Patient Demographics", resource: "Patient", status: "active", @@ -288,6 +289,7 @@ export const mockViewDefinition1 = { export const mockViewDefinition2 = { resourceType: "ViewDefinition", id: "observation-vitals", + url: "https://pathling.example/ViewDefinition/ObservationVitals", name: "Observation Vitals", resource: "Observation", status: "active", @@ -381,7 +383,7 @@ export const mockSqlQueryLibrary1 = { { type: "depends-on", label: "patients", - resource: "ViewDefinition/patient-demographics", + resource: "https://pathling.example/ViewDefinition/PatientDemographics", }, ], parameter: [{ name: "patient_id", use: "in", type: "string" }], @@ -415,6 +417,72 @@ export const mockEmptySqlQueryLibraryBundle: Bundle = { entry: [], }; +// ============================================================================ +// SQLView (sql-view) Library mocks +// ============================================================================ + +/** + * Mock SQLView Library for use in `$sqlquery-run` / `$sqlquery-export` tests. + * + * Carries Base64-encoded SQL ("SELECT 2"), references a single ViewDefinition + * as a dependency and declares no parameters (SQLViews are parameter-less). + */ +export const mockSqlViewLibrary1 = { + resourceType: "Library", + id: "view-active-patients", + status: "active", + title: "Active patients", + type: { + coding: [ + { + system: "https://sql-on-fhir.org/ig/CodeSystem/LibraryTypesCodes", + code: "sql-view", + }, + ], + }, + content: [ + { + contentType: "application/sql", + data: "U0VMRUNUIDI=", + }, + ], + relatedArtifact: [ + { + type: "depends-on", + label: "patients", + resource: "https://pathling.example/ViewDefinition/PatientDemographics", + }, + ], +}; + +/** + * Mock Bundle containing SQLView Library search results. + */ +export const mockSqlViewLibraryBundle: Bundle = { + resourceType: "Bundle", + type: "searchset", + total: 1, + entry: [ + { + resource: mockSqlViewLibrary1 as Bundle["entry"] extends (infer T)[] + ? T extends { resource?: infer R } + ? R + : never + : never, + }, + ], +}; + +/** + * Mock empty SQLView Library Bundle for testing the empty-views state. + */ +export const mockEmptySqlViewLibraryBundle: Bundle = { + resourceType: "Bundle", + type: "searchset", + total: 0, + entry: [], +}; + /** * Mock CSV body for `$sqlquery-run` results. */ diff --git a/ui/e2e/sqlQuery.spec.ts b/ui/e2e/sqlQuery.spec.ts index 2124e75176..e571c14c8d 100644 --- a/ui/e2e/sqlQuery.spec.ts +++ b/ui/e2e/sqlQuery.spec.ts @@ -26,10 +26,13 @@ import { expect, test } from "@playwright/test"; import { mockCapabilityStatement, mockEmptySqlQueryLibraryBundle, + mockEmptySqlViewLibraryBundle, mockSqlQueryLibrary1, mockSqlQueryLibraryBundle, mockSqlQueryRunCsv, mockSqlQueryRunOperationOutcome, + mockSqlViewLibrary1, + mockSqlViewLibraryBundle, mockViewDefinitionBundle, } from "./fixtures/fhirData"; @@ -52,35 +55,34 @@ async function mockMetadata(page: Page) { /** * Mocks the Library search endpoint, branching on the type filter so the - * SQLQuery search returns the SQLQuery bundle while other Library - * searches return an empty bundle. + * SQLQuery search returns the SQLQuery bundle and the SQLView search returns + * the SQLView bundle, while any other Library search returns an empty bundle. * * @param page - The Playwright Page to attach the route to. - * @param bundle - The Bundle to return for SQLQuery searches. + * @param queryBundle - The Bundle to return for SQLQuery searches. + * @param viewBundle - The Bundle to return for SQLView searches. */ async function mockSqlQueryLibraries( page: Page, - bundle: object = mockSqlQueryLibraryBundle, + queryBundle: object = mockSqlQueryLibraryBundle, + viewBundle: object = mockSqlViewLibraryBundle, ) { await page.route(/\/Library\?[^"]*$/, async (route) => { const url = route.request().url(); - if (url.includes("sql-query")) { - await route.fulfill({ - status: 200, - contentType: "application/fhir+json", - body: JSON.stringify(bundle), - }); - return; - } + const bundle = url.includes("sql-view") + ? viewBundle + : url.includes("sql-query") + ? queryBundle + : { + resourceType: "Bundle", + type: "searchset", + total: 0, + entry: [], + }; await route.fulfill({ status: 200, contentType: "application/fhir+json", - body: JSON.stringify({ - resourceType: "Bundle", - type: "searchset", - total: 0, - entry: [], - }), + body: JSON.stringify(bundle), }); }); } @@ -180,7 +182,7 @@ test.describe("SQL on FHIR page - SQL query mode", () => { await selectSqlQueryMode(page); // Pick the stored library. - await page.getByRole("combobox", { name: /sql query library/i }).click(); + await page.getByRole("combobox", { name: /sql query source/i }).click(); await page .getByRole("option", { name: mockSqlQueryLibrary1.title }) .click(); @@ -203,9 +205,60 @@ test.describe("SQL on FHIR page - SQL query mode", () => { await expect(page.getByRole("cell", { name: "Alice" })).toBeVisible(); }); + test("executes a stored SQLView from the SQL views group", async ({ + page, + }) => { + await mockMetadata(page); + await mockSqlQueryLibraries(page); + await mockViewDefinitions(page); + + // Capture the run request to confirm the SQLView resolves as Library/. + let runBody: { parameter?: Array> } | undefined; + await page.route("**/$sqlquery-run", async (route) => { + runBody = JSON.parse(route.request().postData() ?? "{}"); + await route.fulfill({ + status: 200, + contentType: "text/csv", + body: mockSqlQueryRunCsv, + }); + }); + + await page.goto("/admin/sql-on-fhir"); + await selectSqlQueryMode(page); + + // The picker groups queries and views; pick the SQLView. + await page.getByRole("combobox", { name: /sql query source/i }).click(); + await page.getByRole("option", { name: mockSqlViewLibrary1.title }).click(); + + // A SQLView declares no parameters, so the runtime-params section is absent. + await expect(page.getByText("Runtime parameter values")).toBeHidden(); + + // The dependency heading reads "Views" rather than "Tables". + await expect(page.getByText("Views", { exact: true })).toBeVisible(); + + await page.getByRole("combobox", { name: /output format/i }).click(); + await page.getByRole("option", { name: "csv" }).click(); + + await page.getByRole("button", { name: /^execute$/i }).click(); + + await expect(page.getByText("2 rows")).toBeVisible(); + await expect(page.getByRole("cell", { name: "Alice" })).toBeVisible(); + + const queryReference = runBody?.parameter?.find( + (p) => p.name === "queryReference", + ); + expect(queryReference?.valueReference).toEqual({ + reference: "Library/view-active-patients", + }); + }); + test("authors and executes an inline Library", async ({ page }) => { await mockMetadata(page); - await mockSqlQueryLibraries(page, mockEmptySqlQueryLibraryBundle); + await mockSqlQueryLibraries( + page, + mockEmptySqlQueryLibraryBundle, + mockEmptySqlViewLibraryBundle, + ); await mockViewDefinitions(page); await mockSqlQueryRunCsvResponse(page); @@ -218,14 +271,12 @@ test.describe("SQL on FHIR page - SQL query mode", () => { // Author the SQL. await page.getByRole("textbox", { name: /^sql$/i }).fill("SELECT 1"); - // Add a table row and select the first ViewDefinition. - await page.getByRole("button", { name: /add table/i }).click(); + // Add a view row and select the first ViewDefinition. + await page.getByRole("button", { name: /add view/i }).click(); await page - .getByRole("textbox", { name: /label for table 1/i }) + .getByRole("textbox", { name: /label for view 1/i }) .fill("patients"); - await page - .getByRole("combobox", { name: /view definition for table 1/i }) - .click(); + await page.getByRole("combobox", { name: /source for view 1/i }).click(); await page.getByRole("option", { name: "Patient Demographics" }).click(); // Use CSV output so the result rendering is deterministic. @@ -253,13 +304,11 @@ test.describe("SQL on FHIR page - SQL query mode", () => { .getByRole("textbox", { name: /library title/i }) .fill("Inline SQL query"); await page.getByRole("textbox", { name: /^sql$/i }).fill("SELECT 1"); - await page.getByRole("button", { name: /add table/i }).click(); + await page.getByRole("button", { name: /add view/i }).click(); await page - .getByRole("textbox", { name: /label for table 1/i }) + .getByRole("textbox", { name: /label for view 1/i }) .fill("patients"); - await page - .getByRole("combobox", { name: /view definition for table 1/i }) - .click(); + await page.getByRole("combobox", { name: /source for view 1/i }).click(); await page.getByRole("option", { name: "Patient Demographics" }).click(); await page.getByRole("button", { name: /save to server/i }).click(); @@ -270,6 +319,60 @@ test.describe("SQL on FHIR page - SQL query mode", () => { ).toHaveAttribute("aria-selected", "true"); }); + test("saves the chosen source by its canonical URL", async ({ page }) => { + await mockMetadata(page); + await mockViewDefinitions(page); + + // Capture the body POSTed to /Library so the persisted reference can be + // asserted to be the source's canonical URL. + let postedBody: string | null = null; + await page.route(/\/Library$/, async (route) => { + if (route.request().method() === "POST") { + postedBody = route.request().postData(); + await route.fulfill({ + status: 201, + contentType: "application/fhir+json", + body: JSON.stringify({ + ...mockSqlQueryLibrary1, + id: "saved-library", + }), + }); + return; + } + await route.fulfill({ + status: 200, + contentType: "application/fhir+json", + body: JSON.stringify(mockSqlQueryLibraryBundle), + }); + }); + + await page.goto("/admin/sql-on-fhir"); + await selectSqlQueryMode(page); + + await page.getByRole("tab", { name: /provide sql/i }).click(); + await page.getByRole("textbox", { name: /library title/i }).fill("By URL"); + await page.getByRole("textbox", { name: /^sql$/i }).fill("SELECT 1"); + await page.getByRole("button", { name: /add view/i }).click(); + await page + .getByRole("textbox", { name: /label for view 1/i }) + .fill("patients"); + await page.getByRole("combobox", { name: /source for view 1/i }).click(); + await page.getByRole("option", { name: "Patient Demographics" }).click(); + + await page.getByRole("button", { name: /save to server/i }).click(); + + await expect( + page.getByRole("tab", { name: /select query/i }), + ).toHaveAttribute("aria-selected", "true"); + expect(postedBody).not.toBeNull(); + const saved = JSON.parse(postedBody as unknown as string) as { + relatedArtifact?: Array<{ resource?: string }>; + }; + expect(saved.relatedArtifact?.[0]?.resource).toBe( + "https://pathling.example/ViewDefinition/PatientDemographics", + ); + }); + test("renders a callout when the server returns 400", async ({ page }) => { await mockMetadata(page); await mockSqlQueryLibraries(page); @@ -279,7 +382,7 @@ test.describe("SQL on FHIR page - SQL query mode", () => { await page.goto("/admin/sql-on-fhir"); await selectSqlQueryMode(page); - await page.getByRole("combobox", { name: /sql query library/i }).click(); + await page.getByRole("combobox", { name: /sql query source/i }).click(); await page .getByRole("option", { name: mockSqlQueryLibrary1.title }) .click(); diff --git a/ui/e2e/sqlQueryExport.spec.ts b/ui/e2e/sqlQueryExport.spec.ts index 720d2c322e..5efeedef92 100644 --- a/ui/e2e/sqlQueryExport.spec.ts +++ b/ui/e2e/sqlQueryExport.spec.ts @@ -148,7 +148,7 @@ async function runStoredQuery(page: Page) { await page.goto("/admin/sql-on-fhir"); await page.getByRole("tab", { name: /^sql query$/i }).click(); - await page.getByRole("combobox", { name: /sql query library/i }).click(); + await page.getByRole("combobox", { name: /sql query source/i }).click(); await page.getByRole("option", { name: mockSqlQueryLibrary1.title }).click(); // Enter a runtime value for the declared parameter. diff --git a/ui/src/api/__tests__/sqlQuery.test.ts b/ui/src/api/__tests__/sqlQuery.test.ts index 3cd2d097fd..2e0ab34ab9 100644 --- a/ui/src/api/__tests__/sqlQuery.test.ts +++ b/ui/src/api/__tests__/sqlQuery.test.ts @@ -24,7 +24,9 @@ import { } from "../../types/errors"; import { listSqlQueryLibraries, + listStoredLibraries, SQL_QUERY_LIBRARY_TYPE_FILTER, + SQL_VIEW_LIBRARY_TYPE_FILTER, sqlQueryExportDownload, sqlQueryExportKickOff, sqlQueryRun, @@ -373,6 +375,88 @@ describe("listSqlQueryLibraries", () => { }); }); +describe("SQL_VIEW_LIBRARY_TYPE_FILTER", () => { + // The SQLView filter shares the SQL on FHIR Library code system with the + // SQLQuery filter; only the type code differs. + it("is the SQL on FHIR Library code system scoped to sql-view", () => { + expect(SQL_VIEW_LIBRARY_TYPE_FILTER).toBe( + "https://sql-on-fhir.org/ig/CodeSystem/LibraryTypesCodes|sql-view", + ); + }); +}); + +describe("listStoredLibraries", () => { + // The generalised list function scopes its search by the requested type + // code, issuing the SQLView token for sql-view requests. + it("queries Library with the sql-view type filter", async () => { + mockFetch.mockResolvedValueOnce( + new Response(JSON.stringify({ resourceType: "Bundle", entry: [] }), { + status: 200, + headers: { "Content-Type": "application/fhir+json" }, + }), + ); + + await listStoredLibraries("https://example.com/fhir", { + typeCode: "sql-view", + }); + + const url = decodeURIComponent(mockFetch.mock.calls[0][0] as string); + expect(url).toContain("/Library?"); + expect(url).toContain(`type=${SQL_VIEW_LIBRARY_TYPE_FILTER}`); + }); + + // The same function issues the SQLQuery token for sql-query requests, so + // both Library kinds share one code path. + it("queries Library with the sql-query type filter", async () => { + mockFetch.mockResolvedValueOnce( + new Response(JSON.stringify({ resourceType: "Bundle", entry: [] }), { + status: 200, + headers: { "Content-Type": "application/fhir+json" }, + }), + ); + + await listStoredLibraries("https://example.com/fhir", { + typeCode: "sql-query", + }); + + const url = decodeURIComponent(mockFetch.mock.calls[0][0] as string); + expect(url).toContain(`type=${SQL_QUERY_LIBRARY_TYPE_FILTER}`); + }); + + // The Authorization header is forwarded for authenticated servers. + it("attaches a bearer token when an access token is provided", async () => { + mockFetch.mockResolvedValueOnce( + new Response(JSON.stringify({ resourceType: "Bundle" }), { + status: 200, + headers: { "Content-Type": "application/fhir+json" }, + }), + ); + + await listStoredLibraries("https://example.com/fhir", { + typeCode: "sql-view", + accessToken: "secret", + }); + + expect(mockFetch).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ + headers: expect.objectContaining({ Authorization: "Bearer secret" }), + }), + ); + }); + + // A 401 propagates as UnauthorizedError, consistent with the rest of the + // API layer. + it("throws UnauthorizedError on a 401 response", async () => { + mockFetch.mockResolvedValueOnce( + new Response("Unauthorized", { status: 401 }), + ); + await expect( + listStoredLibraries("https://example.com/fhir", { typeCode: "sql-view" }), + ).rejects.toThrow(UnauthorizedError); + }); +}); + describe("sqlQueryExportKickOff", () => { // The kick-off must request asynchronous processing and return the polling // URL carried by the Content-Location header. diff --git a/ui/src/api/index.ts b/ui/src/api/index.ts index 5bac26cc9e..9a3549f594 100644 --- a/ui/src/api/index.ts +++ b/ui/src/api/index.ts @@ -64,13 +64,16 @@ export type { ViewDefinition } from "./view"; export { sqlQueryRun, listSqlQueryLibraries, + listStoredLibraries, sqlQueryExportKickOff, sqlQueryExportDownload, SQL_QUERY_LIBRARY_TYPE_SYSTEM, SQL_QUERY_LIBRARY_TYPE_FILTER, + SQL_VIEW_LIBRARY_TYPE_FILTER, SQL_QUERY_LIBRARY_PROFILE, } from "./sqlQuery"; export type { + SqlOnFhirLibraryTypeCode, SqlQueryRunOptions, SqlQueryRunStoredOptions, SqlQueryRunInlineOptions, diff --git a/ui/src/api/sqlQuery.ts b/ui/src/api/sqlQuery.ts index e8fc0cf310..39c058fd6d 100644 --- a/ui/src/api/sqlQuery.ts +++ b/ui/src/api/sqlQuery.ts @@ -17,7 +17,7 @@ /** * Client for the SQL on FHIR `$sqlquery-run` operation and the search - * endpoint that lists stored SQLQuery Library resources. + * endpoint that lists stored SQLQuery and SQLView Library resources. * * @author John Grimes */ @@ -51,6 +51,27 @@ export const SQL_QUERY_LIBRARY_TYPE_SYSTEM = */ export const SQL_QUERY_LIBRARY_TYPE_FILTER = `${SQL_QUERY_LIBRARY_TYPE_SYSTEM}|sql-query`; +/** + * Token-search filter that scopes a Library search to the SQLView profile. + * + * Shares the code system with {@link SQL_QUERY_LIBRARY_TYPE_FILTER}; only the + * type code differs. + */ +export const SQL_VIEW_LIBRARY_TYPE_FILTER = `${SQL_QUERY_LIBRARY_TYPE_SYSTEM}|sql-view`; + +/** + * SQL on FHIR Library type codes that the UI can list and run. + */ +export type SqlOnFhirLibraryTypeCode = "sql-query" | "sql-view"; + +/** + * Maps each listable Library type code to its token-search filter. + */ +const LIBRARY_TYPE_FILTERS: Record = { + "sql-query": SQL_QUERY_LIBRARY_TYPE_FILTER, + "sql-view": SQL_VIEW_LIBRARY_TYPE_FILTER, +}; + /** * Profile URL applied to inline SQLQuery Library resources. */ @@ -189,27 +210,32 @@ export async function sqlQueryRun( } /** - * Searches the FHIR server for stored SQLQuery Library resources. + * Searches the FHIR server for stored SQL on FHIR Library resources of a + * given type. * * Uses a `type` token search scoped to the SQL on FHIR Library type code - * system and the `sql-query` code, so unrelated Library resources are - * excluded. + * system and the requested code (`sql-query` or `sql-view`), so unrelated + * Library resources are excluded. SQLQueries and SQLViews are both Library + * resources distinguished only by this code, so a single function lists + * either kind. * * @param baseUrl - The FHIR server base URL. - * @param options - Optional auth configuration. + * @param options - The type code to list, plus optional auth configuration. * @returns A FHIR Bundle containing the matched Library resources. * @throws {UnauthorizedError} When the request receives a 401 response. * @throws {Error} For other non-successful responses. * * @example - * const bundle = await listSqlQueryLibraries("https://example.com/fhir"); + * const bundle = await listStoredLibraries("https://example.com/fhir", { + * typeCode: "sql-view", + * }); */ -export async function listSqlQueryLibraries( +export async function listStoredLibraries( baseUrl: string, - options: AuthOptions = {}, + options: { typeCode: SqlOnFhirLibraryTypeCode } & AuthOptions, ): Promise { const url = buildUrl(baseUrl, "/Library", { - type: SQL_QUERY_LIBRARY_TYPE_FILTER, + type: LIBRARY_TYPE_FILTERS[options.typeCode], }); const headers = buildHeaders({ accessToken: options.accessToken }); @@ -218,6 +244,31 @@ export async function listSqlQueryLibraries( return (await response.json()) as Bundle; } +/** + * Searches the FHIR server for stored SQLQuery Library resources. + * + * A thin wrapper over {@link listStoredLibraries} scoped to the `sql-query` + * type code, retained for the existing callers. + * + * @param baseUrl - The FHIR server base URL. + * @param options - Optional auth configuration. + * @returns A FHIR Bundle containing the matched Library resources. + * @throws {UnauthorizedError} When the request receives a 401 response. + * @throws {Error} For other non-successful responses. + * + * @example + * const bundle = await listSqlQueryLibraries("https://example.com/fhir"); + */ +export async function listSqlQueryLibraries( + baseUrl: string, + options: AuthOptions = {}, +): Promise { + return listStoredLibraries(baseUrl, { + typeCode: "sql-query", + accessToken: options.accessToken, + }); +} + /** * Builds the nested `parameters` Parameters resource carrying runtime * bindings, or returns `undefined` if no non-empty bindings exist. diff --git a/ui/src/components/sqlOnFhir/SqlPreview.tsx b/ui/src/components/sqlOnFhir/SqlPreview.tsx new file mode 100644 index 0000000000..c6bf00fe21 --- /dev/null +++ b/ui/src/components/sqlOnFhir/SqlPreview.tsx @@ -0,0 +1,83 @@ +/* + * 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. + */ + +/** + * Shared, height-bounded SQL display. + * + * Renders SQL text in a read-only, scrollable text area with an overlaid copy + * control. Used both for previewing the SQL of a stored query or view in the + * form and for echoing the submitted SQL in a result card, so the two surfaces + * stay visually consistent and neither grows unbounded with long queries. + * + * @author John Grimes + */ + +import { CopyIcon } from "@radix-ui/react-icons"; +import { Box, IconButton, TextArea, Tooltip } from "@radix-ui/themes"; + +import { useClipboard } from "../../hooks"; + +interface SqlPreviewProps { + /** The SQL text to display. */ + sql: string; + /** Accessible label for the read-only text area. */ + ariaLabel: string; + /** Number of visible text rows before the area scrolls. Defaults to 10. */ + rows?: number; +} + +/** + * Renders SQL in a read-only, height-bounded text area with a copy control. + * + * @param props - The component props. + * @param props.sql - The SQL text to display. + * @param props.ariaLabel - Accessible label for the read-only text area. + * @param props.rows - Number of visible text rows before scrolling. Defaults to 10. + * @returns The SQL preview element. + */ +export function SqlPreview({ sql, ariaLabel, rows = 10 }: Readonly) { + const copyToClipboard = useClipboard(); + + return ( + + + copyToClipboard(sql)} + style={{ + position: "absolute", + top: 8, + right: 8, + zIndex: 1, + }} + > + + + +