diff --git a/.github/workflows/contract-tests.yml b/.github/workflows/contract-tests.yml index bdc19096..9a3c26f7 100644 --- a/.github/workflows/contract-tests.yml +++ b/.github/workflows/contract-tests.yml @@ -5,12 +5,24 @@ on: branches: - main - release/* + - flowvault-release/* jobs: contract-tests: - name: Contract Tests + # One job per module so a break in one is reported against that module by name, + # and both still run even when the other fails. + name: Contract Tests (${{ matrix.module }}) runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + include: + - module: skyvault + artifact: skyflow-java + - module: flowvault + artifact: skyflow-flowvault-java + permissions: contents: read pull-requests: write @@ -29,29 +41,31 @@ jobs: cache: 'maven' - name: Verify API surface snapshot - run: mvn -B install -pl common,skyvault -am -DskipTests -Dmaven.javadoc.skip=true -Dgpg.skip=true + run: mvn -B install -pl common,${{ matrix.module }} -am -DskipTests -Dmaven.javadoc.skip=true -Dgpg.skip=true - name: Show API surface diff if: failure() run: | - echo "### API surface changes detected ###" - echo "See skyvault/target/japicmp/default-cli.diff for the full comparison against skyvault/api-report/skyflow-java.baseline.jar." - echo "If this change is intentional, run scripts/contract-snapshot-update.sh and commit the updated baseline jar." + echo "### API surface changes detected in ${{ matrix.module }} ###" + echo "Compared against ${{ matrix.module }}/api-report/${{ matrix.artifact }}.baseline.jar." + echo "If this change is intentional, run:" + echo " scripts/contract-snapshot-update.sh ${{ matrix.module }}" + echo "and commit the updated baseline jar." echo "" - cat skyvault/target/japicmp/default-cli.diff || true + cat ${{ matrix.module }}/target/japicmp/default-cli.diff || true - name: Upload API surface diff on failure if: failure() uses: actions/upload-artifact@v4 with: - name: api-surface-diff - path: skyvault/target/japicmp/** + name: api-surface-diff-${{ matrix.module }} + path: ${{ matrix.module }}/target/japicmp/** retention-days: 7 # The step above only shows a diff when the CURRENT build differs from the # committed baseline - once someone runs contract-snapshot-update.sh and # commits the refreshed baseline jar, that check goes green and shows nothing. - # A reviewer looking at a green PR that touches api-report/skyflow-java.baseline.jar + # A reviewer looking at a green PR that touches api-report/*.baseline.jar # (a binary file) would otherwise have no way to see WHAT was just approved as # the new contract. These steps explicitly diff the OLD committed baseline # (from the PR's base branch) against the NEW committed baseline (from this PR) @@ -61,40 +75,74 @@ jobs: if: always() && github.event.pull_request run: | git fetch origin "${{ github.event.pull_request.base.ref }}" --depth=1 - if git diff --name-only "origin/${{ github.event.pull_request.base.ref }}" HEAD -- skyvault/api-report/skyflow-java.baseline.jar | grep -q .; then + BASELINE="${{ matrix.module }}/api-report/${{ matrix.artifact }}.baseline.jar" + if ! git diff --name-only "origin/${{ github.event.pull_request.base.ref }}" HEAD -- "$BASELINE" | grep -q .; then + echo "changed=false" >> "$GITHUB_OUTPUT" + elif git cat-file -e "origin/${{ github.event.pull_request.base.ref }}:$BASELINE" 2>/dev/null; then echo "changed=true" >> "$GITHUB_OUTPUT" else - echo "changed=false" >> "$GITHUB_OUTPUT" + # Added by this PR rather than modified: the module is getting its + # first baseline. git diff reports an addition as a change, but there + # is no old snapshot to `git show`, so a plain "true" here would send + # the next step into `git show :` and exit 128. + echo "changed=new" >> "$GITHUB_OUTPUT" fi - name: Diff old vs new contract baseline - if: always() && steps.baseline-diff-check.outputs.changed == 'true' + if: always() && (steps.baseline-diff-check.outputs.changed == 'true' || steps.baseline-diff-check.outputs.changed == 'new') run: | + BASELINE="${{ matrix.module }}/api-report/${{ matrix.artifact }}.baseline.jar" + + if [ "${{ steps.baseline-diff-check.outputs.changed }}" = "new" ]; then + { + echo "\`$BASELINE\` is **new in this PR** - \`${{ matrix.module }}\` had no committed baseline before, so there is nothing to diff against." + echo "" + echo "This snapshot becomes the approved contract: every later PR is compared against it, and any incompatible change fails the \`Contract Tests (${{ matrix.module }})\` job until someone regenerates it deliberately. Review it as the starting point, not as a change." + } > /tmp/contract-baseline-diff.md + cat /tmp/contract-baseline-diff.md + exit 0 + fi + curl -sL -o /tmp/japicmp-cli.jar "https://repo.maven.apache.org/maven2/com/github/siom79/japicmp/japicmp/0.26.0/japicmp-0.26.0-jar-with-dependencies.jar" - mvn -q -B dependency:build-classpath -pl skyvault -Dmdep.outputFile=/tmp/skyvault-classpath.txt -Dmaven.javadoc.skip=true -Dgpg.skip=true - git show "origin/${{ github.event.pull_request.base.ref }}:skyvault/api-report/skyflow-java.baseline.jar" > /tmp/old-baseline.jar + mvn -q -B dependency:build-classpath -pl ${{ matrix.module }} -Dmdep.outputFile=/tmp/module-classpath.txt -Dmaven.javadoc.skip=true -Dgpg.skip=true + git show "origin/${{ github.event.pull_request.base.ref }}:$BASELINE" > /tmp/old-baseline.jar + # Same allowlist the poms gate on, so the comment shows the contract and + # nothing else. Keep these in sync with the in the module poms. java -jar /tmp/japicmp-cli.jar \ -o /tmp/old-baseline.jar \ - -n skyvault/api-report/skyflow-java.baseline.jar \ + -n "$BASELINE" \ -a protected \ - -e "com.skyflow.generated.*;com.skyflow.utils.*" \ - --old-classpath "$(cat /tmp/skyvault-classpath.txt)" \ - --new-classpath "$(cat /tmp/skyvault-classpath.txt)" \ + -i "com.skyflow.Skyflow;com.skyflow.config;com.skyflow.enums;com.skyflow.errors;com.skyflow.serviceaccount.util;com.skyflow.vault.audit;com.skyflow.vault.bin;com.skyflow.vault.connection;com.skyflow.vault.controller;com.skyflow.vault.data;com.skyflow.vault.detect;com.skyflow.vault.tokens" \ + --old-classpath "$(cat /tmp/module-classpath.txt)" \ + --new-classpath "$(cat /tmp/module-classpath.txt)" \ -m \ + --ignore-missing-classes \ --markdown > /tmp/contract-baseline-diff.md || true cat /tmp/contract-baseline-diff.md - name: Comment contract baseline change on PR - if: always() && steps.baseline-diff-check.outputs.changed == 'true' + if: always() && (steps.baseline-diff-check.outputs.changed == 'true' || steps.baseline-diff-check.outputs.changed == 'new') uses: actions/github-script@v7 + env: + BASELINE_STATE: ${{ steps.baseline-diff-check.outputs.changed }} with: script: | const fs = require('fs'); + const module = '${{ matrix.module }}'; + const artifact = '${{ matrix.artifact }}'; const summary = fs.readFileSync('/tmp/contract-baseline-diff.md', 'utf8'); - const marker = ''; - const body = `${marker}\n## Contract baseline change detected\n\nThis PR updates \`skyvault/api-report/skyflow-java.baseline.jar\` (the approved public API contract). Here is exactly what it changes, comparing the baseline on \`${{ github.event.pull_request.base.ref }}\` against the baseline committed in this PR:\n\n${summary}`; + // per-module marker so the two matrix jobs update their own comment + const marker = ``; + const isNew = process.env.BASELINE_STATE === 'new'; + const heading = isNew + ? `## Contract baseline added (\`${module}\`)` + : `## Contract baseline change detected (\`${module}\`)`; + const preamble = isNew + ? `This PR adds \`${module}/api-report/${artifact}.baseline.jar\`, the approved public API contract for this module.` + : `This PR updates \`${module}/api-report/${artifact}.baseline.jar\` (the approved public API contract). Here is exactly what it changes, comparing the baseline on \`${{ github.event.pull_request.base.ref }}\` against the baseline committed in this PR:`; + const body = `${marker}\n${heading}\n\n${preamble}\n\n${summary}`; const { data: comments } = await github.rest.issues.listComments({ owner: context.repo.owner, repo: context.repo.repo, diff --git a/common/src/test/java/com/skyflow/vault/data/RequestContextTests.java b/common/src/test/java/com/skyflow/vault/data/RequestContextTests.java deleted file mode 100644 index cc7bd5d7..00000000 --- a/common/src/test/java/com/skyflow/vault/data/RequestContextTests.java +++ /dev/null @@ -1,68 +0,0 @@ -package com.skyflow.vault.data; - -import com.skyflow.enums.CustomHeaderKey; -import org.junit.Assert; -import org.junit.Test; - -import java.util.Map; - -public class RequestContextTests { - - @Test - public void testGetOperationReturnsConstructorValue() { - RequestContext context = new RequestContext("INSERT"); - - Assert.assertEquals("INSERT", context.getOperation()); - } - - @Test - public void testNullOperation() { - RequestContext context = new RequestContext(null); - - Assert.assertNull(context.getOperation()); - } - - @Test - public void testGetHeadersReturnsEmptyMapByDefault() { - RequestContext context = new RequestContext("INSERT"); - - Assert.assertTrue(context.getHeaders().isEmpty()); - } - - @Test - public void testAddHeaderIsReflectedInGetHeaders() { - RequestContext context = new RequestContext("INSERT"); - context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_ID, "account-id-value"); - - Map headers = context.getHeaders(); - - Assert.assertEquals(1, headers.size()); - Assert.assertEquals("account-id-value", headers.get(CustomHeaderKey.SKYFLOW_ACCOUNT_ID)); - } - - @Test - public void testAddHeaderOverwritesExistingValueForSameKey() { - RequestContext context = new RequestContext("INSERT"); - context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_ID, "first-value"); - context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_ID, "second-value"); - - Assert.assertEquals(1, context.getHeaders().size()); - Assert.assertEquals("second-value", context.getHeaders().get(CustomHeaderKey.SKYFLOW_ACCOUNT_ID)); - } - - @Test - public void testAddMultipleDistinctHeaders() { - RequestContext context = new RequestContext("DETOKENIZE"); - context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_ID, "account-id-value"); - context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_NAME, "account-name-value"); - - Assert.assertEquals(2, context.getHeaders().size()); - } - - @Test(expected = UnsupportedOperationException.class) - public void testGetHeadersReturnsUnmodifiableMap() { - RequestContext context = new RequestContext("INSERT"); - - context.getHeaders().put(CustomHeaderKey.REQUEST_ID_HEADER, "request-id-value"); - } -} diff --git a/flowvault/api-report/skyflow-flowvault-java.baseline.jar b/flowvault/api-report/skyflow-flowvault-java.baseline.jar new file mode 100644 index 00000000..d98e0eb7 Binary files /dev/null and b/flowvault/api-report/skyflow-flowvault-java.baseline.jar differ diff --git a/flowvault/pom.xml b/flowvault/pom.xml index 92136399..54545b1d 100644 --- a/flowvault/pom.xml +++ b/flowvault/pom.xml @@ -77,6 +77,96 @@ + + + org.apache.maven.plugins + maven-shade-plugin + 3.6.0 + + + shade-for-japicmp + package + + shade + + + true + with-common + + + com.skyflow:common + + + + + + + + com.github.siom79.japicmp + japicmp-maven-plugin + 0.26.0 + + + + + ${project.basedir}/api-report/skyflow-flowvault-java.baseline.jar + + + + + ${project.build.directory}/${project.build.finalName}-with-common.jar + + + + protected + true + true + true + true + + + com.skyflow.Skyflow + com.skyflow.config + com.skyflow.enums + com.skyflow.errors + com.skyflow.serviceaccount.util + com.skyflow.vault.controller + com.skyflow.vault.data + + false + false + + + + + default-cli + verify + + cmp + + + + diff --git a/common/src/main/java/com/skyflow/enums/CustomHeaderKey.java b/flowvault/src/main/java/com/skyflow/enums/CustomHeaderKey.java similarity index 100% rename from common/src/main/java/com/skyflow/enums/CustomHeaderKey.java rename to flowvault/src/main/java/com/skyflow/enums/CustomHeaderKey.java diff --git a/common/src/main/java/com/skyflow/vault/data/RequestContext.java b/flowvault/src/main/java/com/skyflow/vault/data/RequestContext.java similarity index 100% rename from common/src/main/java/com/skyflow/vault/data/RequestContext.java rename to flowvault/src/main/java/com/skyflow/vault/data/RequestContext.java diff --git a/common/src/main/java/com/skyflow/vault/data/RequestInterceptor.java b/flowvault/src/main/java/com/skyflow/vault/data/RequestInterceptor.java similarity index 100% rename from common/src/main/java/com/skyflow/vault/data/RequestInterceptor.java rename to flowvault/src/main/java/com/skyflow/vault/data/RequestInterceptor.java diff --git a/flowvault/src/test/java/com/skyflow/vault/data/RequestContextTests.java b/flowvault/src/test/java/com/skyflow/vault/data/RequestContextTests.java index 537350af..6d93125d 100644 --- a/flowvault/src/test/java/com/skyflow/vault/data/RequestContextTests.java +++ b/flowvault/src/test/java/com/skyflow/vault/data/RequestContextTests.java @@ -1,11 +1,15 @@ package com.skyflow.vault.data; import com.skyflow.enums.CustomHeaderKey; + +import java.util.Map; import org.junit.Assert; import org.junit.Test; /** - * Batch position on the interceptor context. Without it every batch of a bulk call presents an + * The interceptor context: its operation, its custom headers, and the batch position. + * + *

Batch position matters without it every batch of a bulk call presents an * identical context, so a caller cannot tag them apart — no per-batch correlation id, no * "batch 3 of 12" logging. */ @@ -53,4 +57,63 @@ public void testHeadersRemainUnmodifiable() { Assert.assertTrue(true); } } + + // ── operation and headers (moved here with the class, from common) ────────── + @Test + public void testGetOperationReturnsConstructorValue() { + RequestContext context = new RequestContext("INSERT"); + + Assert.assertEquals("INSERT", context.getOperation()); + } + + @Test + public void testNullOperation() { + RequestContext context = new RequestContext(null); + + Assert.assertNull(context.getOperation()); + } + + @Test + public void testGetHeadersReturnsEmptyMapByDefault() { + RequestContext context = new RequestContext("INSERT"); + + Assert.assertTrue(context.getHeaders().isEmpty()); + } + + @Test + public void testAddHeaderIsReflectedInGetHeaders() { + RequestContext context = new RequestContext("INSERT"); + context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_ID, "account-id-value"); + + Map headers = context.getHeaders(); + + Assert.assertEquals(1, headers.size()); + Assert.assertEquals("account-id-value", headers.get(CustomHeaderKey.SKYFLOW_ACCOUNT_ID)); + } + + @Test + public void testAddHeaderOverwritesExistingValueForSameKey() { + RequestContext context = new RequestContext("INSERT"); + context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_ID, "first-value"); + context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_ID, "second-value"); + + Assert.assertEquals(1, context.getHeaders().size()); + Assert.assertEquals("second-value", context.getHeaders().get(CustomHeaderKey.SKYFLOW_ACCOUNT_ID)); + } + + @Test + public void testAddMultipleDistinctHeaders() { + RequestContext context = new RequestContext("DETOKENIZE"); + context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_ID, "account-id-value"); + context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_NAME, "account-name-value"); + + Assert.assertEquals(2, context.getHeaders().size()); + } + + @Test(expected = UnsupportedOperationException.class) + public void testGetHeadersReturnsUnmodifiableMap() { + RequestContext context = new RequestContext("INSERT"); + + context.getHeaders().put(CustomHeaderKey.REQUEST_ID_HEADER, "request-id-value"); + } } diff --git a/common/src/test/java/com/skyflow/vault/data/RequestInterceptorTests.java b/flowvault/src/test/java/com/skyflow/vault/data/RequestInterceptorTests.java similarity index 100% rename from common/src/test/java/com/skyflow/vault/data/RequestInterceptorTests.java rename to flowvault/src/test/java/com/skyflow/vault/data/RequestInterceptorTests.java diff --git a/scripts/contract-snapshot-update.sh b/scripts/contract-snapshot-update.sh index bd33397b..9c4c5314 100755 --- a/scripts/contract-snapshot-update.sh +++ b/scripts/contract-snapshot-update.sh @@ -1,27 +1,56 @@ #!/usr/bin/env bash -# Regenerates the skyvault contract-testing baseline (api-report/skyflow-java.baseline.jar) +# Regenerates a module's contract-testing baseline (/api-report/*.baseline.jar) # from the CURRENT working tree and overwrites the committed snapshot. # -# Run this after an intentional public API change to v2, review the resulting -# git diff on the jar (a new binary blob) alongside your code change, and commit -# both together. This is the only way the committed baseline should ever change - -# japicmp never pulls a published version for this comparison. +# Run this after an intentional public API change, review the resulting git diff on +# the jar (a new binary blob) alongside your code change, and commit both together. +# This is the only way a committed baseline should ever change - japicmp never pulls +# a published version for the comparison. +# +# scripts/contract-snapshot-update.sh skyvault # regenerate one module +# scripts/contract-snapshot-update.sh flowvault +# scripts/contract-snapshot-update.sh # both +# +# Prefer naming the module you actually changed. Jar archives embed timestamps, so +# regenerating a module whose API did not change still produces different bytes and +# a spurious diff on a binary file - which is exactly the thing a reviewer cannot +# eyeball. Only the baseline you intend to move should appear in the commit. set -euo pipefail cd "$(dirname "$0")/.." -mvn -B package -pl common,v2 -am -DskipTests -Dmaven.javadoc.skip=true -Dgpg.skip=true - -SHADED_JAR=$(ls skyvault/target/skyflow-java-*-with-common.jar | head -n1) +# module -> artifactId, which is also the baseline jar's name +declare -A ARTIFACTS=( + [skyvault]="skyflow-java" + [flowvault]="skyflow-flowvault-java" +) -if [ -z "$SHADED_JAR" ]; then - echo "Error: could not find a built skyvault/target/skyflow-java-*-with-common.jar. Did the build succeed?" - exit 1 +MODULES=("$@") +if [ ${#MODULES[@]} -eq 0 ]; then + MODULES=(skyvault flowvault) fi -mkdir -p skyvault/api-report -cp "$SHADED_JAR" skyvault/api-report/skyflow-java.baseline.jar +for MODULE in "${MODULES[@]}"; do + ARTIFACT="${ARTIFACTS[$MODULE]:-}" + if [ -z "$ARTIFACT" ]; then + echo "Error: unknown module '$MODULE'. Expected one of: ${!ARTIFACTS[*]}" + exit 1 + fi + + echo "=== $MODULE ===" + mvn -B package -pl "common,$MODULE" -am -DskipTests -Dmaven.javadoc.skip=true -Dgpg.skip=true + + # the comparison-only jar, which merges com.skyflow:common into the module + SHADED_JAR=$(ls "$MODULE"/target/"$ARTIFACT"-*-with-common.jar 2>/dev/null | head -n1) + if [ -z "$SHADED_JAR" ]; then + echo "Error: could not find $MODULE/target/$ARTIFACT-*-with-common.jar. Did the build succeed?" + exit 1 + fi + + mkdir -p "$MODULE/api-report" + cp "$SHADED_JAR" "$MODULE/api-report/$ARTIFACT.baseline.jar" + echo "Updated $MODULE/api-report/$ARTIFACT.baseline.jar from $SHADED_JAR" +done echo "--------------------------" -echo "Updated skyvault/api-report/skyflow-java.baseline.jar from $SHADED_JAR" -echo "Review the diff and commit this file alongside your API change." +echo "Review the diff and commit the baseline jar(s) alongside your API change." diff --git a/skyvault/api-report/skyflow-java.baseline.jar b/skyvault/api-report/skyflow-java.baseline.jar index 6911c011..81085660 100644 Binary files a/skyvault/api-report/skyflow-java.baseline.jar and b/skyvault/api-report/skyflow-java.baseline.jar differ diff --git a/skyvault/pom.xml b/skyvault/pom.xml index 42fde4e5..618c7e9d 100644 --- a/skyvault/pom.xml +++ b/skyvault/pom.xml @@ -119,10 +119,35 @@ false true true - - com.skyflow.generated.* - com.skyflow.utils.* - + + + com.skyflow.Skyflow + com.skyflow.config + com.skyflow.enums + com.skyflow.errors + com.skyflow.serviceaccount.util + com.skyflow.vault.audit + com.skyflow.vault.bin + com.skyflow.vault.connection + com.skyflow.vault.controller + com.skyflow.vault.data + com.skyflow.vault.detect + com.skyflow.vault.tokens + false false