Skip to content

Updates for Senzing 4.3 staging on Darwin using /opt/homebrew/lib in library path#227

Merged
barrycaceres merged 12 commits intomainfrom
caceres-homebrew-libs-1
Mar 23, 2026
Merged

Updates for Senzing 4.3 staging on Darwin using /opt/homebrew/lib in library path#227
barrycaceres merged 12 commits intomainfrom
caceres-homebrew-libs-1

Conversation

@barrycaceres
Copy link
Copy Markdown
Contributor

@barrycaceres barrycaceres commented Mar 5, 2026

  • Updated test-runs to use /opt/homebrew/lib
  • Updated Darwin Github workflow to use /opt/homebrew/lib
  • Updated Java formatting and styling rules
  • Fixed SQLiteUri.parse() to handle file paths containing spaces on non-Windows
    platforms (e.g.: macOS ~/Library/Application Support/). Previously, the
    non-Windows code path routed through java.net.URI which rejected unencoded
    spaces with URISyntaxException.
  • Fixed SQLiteUri.toString() to emit raw file paths without URL encoding on
    non-Windows platforms. Previously, urlEncodeUtf8() encoded spaces as +
    (form-encoding) which is invalid for URI paths and not recognized by the
    native Senzing engine.
  • Added path validation in SQLiteUri.parse() for non-Windows paths to reject
    backslashes and require absolute (/) or explicitly relative (./, ../)
    path prefixes.
  • Added unit tests for non-Windows file paths containing spaces.
  • Updated dependencies:
    • Updated jackson-bom from version 2.21.0 to 2.21.2
    • Updated armeria-bom from version 1.36.0 to 1.37.0
    • Updated netty-bom from version 4.2.9.Final to 4.2.10.Final
    • Updated sqlite-jdbc from version 3.51.2.0 to 3.51.3.0
    • Updated amqp-client from version 5.28.0 to 5.29.0
    • Updated Amazon sqs from version 2.41.28 to 2.42.18
    • Updated embedded-postgres from version 2.2.0 to 2.2.2
    • Removed explicit version for jackson-datatype-joda (now managed by jackson-bom)
    • Updated maven-resources-plugin from version 3.4.0 to 3.5.0
    • Updated maven-shade-plugin from version 3.6.1 to 3.6.2
    • Updated maven-surefire-plugin from version 3.5.4 to 3.5.5

@barrycaceres barrycaceres requested review from a team as code owners March 5, 2026 00:12
@barrycaceres barrycaceres self-assigned this Mar 5, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 5, 2026

🤖 Claude Code Review

PR Code Review

Code Quality

✅ No commented-out code — No commented-out code found in the diff.

✅ Meaningful variable names — Variable names are clear and descriptive.

✅ DRY principle followed — The /opt/homebrew/lib path appears in two places (.github/workflows/maven-darwin.yaml and src/test/java/com/senzing/test/GenerateTestJVMScript.java), which is a minor duplication but acceptable given these are different contexts (CI workflow vs. test JVM script generator).

⚠️ Defects / Logic Concerns:

  • .github/workflows/maven-darwin.yaml line 50 / GenerateTestJVMScript.java line 158: The hardcoded path /opt/homebrew/lib assumes Homebrew is always installed at the default location. This will fail on Intel Macs (where Homebrew installs to /usr/local/lib) or in environments where Homebrew is installed to a custom prefix. The previous value ${SENZING_PATH}/er/lib/macos was self-contained. Consider using $(brew --prefix)/lib in the workflow script, or making the path configurable.

  • GenerateTestJVMScript.java line 158: The original code used new File(libDir, "macos") which made platformLibDir relative to the Senzing installation. The replacement new File("/opt/homebrew/lib") is an absolute hardcoded path that breaks portability for the same reasons as above. Also note that libDir is still set on line 157 but platformLibDir no longer uses it, creating a minor inconsistency.

✅ Style guide — The checkstyle.xml addition and checkstyle-suppressions.xml changes enforce the project's code style (Allman brace style, 80-char line limit, proper indentation). This is consistent with the Senzing style guide.

✅ No security vulnerabilities — No injection or security issues found.


Testing

❌ No tests for the path change — The change to GenerateTestJVMScript.java modifies a utility that generates JVM launch scripts for tests. There are no accompanying tests or verification that the new hardcoded path works correctly across all supported macOS environments (Intel vs Apple Silicon). The previous path was relative to the Senzing installation directory; the new one is absolute.

N/A — No new functions or API endpoints introduced.


Documentation

✅ No README changes required — This is a tooling/configuration change.

✅ Inline comments — The checkstyle.xml file has clear inline comments explaining each rule.

❌ CHANGELOG.md — No CHANGELOG.md update is included. If the project maintains a changelog, this should document the formatter configuration addition and the macOS library path fix.


Security

✅ No hardcoded credentials — None found.

✅ No sensitive data in logs — Not applicable to this diff.

✅ No license files — No .lic files or AQAAAD-prefixed content checked in.


Summary

Item Status Notes
Style guide compliance Checkstyle config enforces project style
No commented-out code Clean
Meaningful variable names Clear
DRY principle Minor duplication is acceptable
Defects/logic errors Hardcoded /opt/homebrew/lib breaks Intel Macs
Unit tests No test coverage for path change
Documentation/CHANGELOG No CHANGELOG update
Security No issues

Critical Issue: The hardcoded /opt/homebrew/lib path in both .github/workflows/maven-darwin.yaml:50 and GenerateTestJVMScript.java:158 assumes Apple Silicon Homebrew layout. On Intel Macs, Homebrew uses /usr/local/lib. The workflow should use $(brew --prefix)/lib to resolve the correct path dynamically, and the Java file should either use a system property or environment variable, or call brew --prefix at runtime.

Automated code review analyzing defects and coding standards

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 5, 2026

Super-linter summary

Language Validation result
CHECKOV Pass ✅
GITHUB_ACTIONS Pass ✅
GITHUB_ACTIONS_ZIZMOR Pass ✅
GITLEAKS Pass ✅
GIT_MERGE_CONFLICT_MARKERS Pass ✅
JSCPD Pass ✅
PRE_COMMIT Pass ✅
SPELL_CODESPELL Pass ✅
TRIVY Fail ❌
YAML Pass ✅
YAML_PRETTIER Pass ✅

Super-linter detected linting errors

For more information, see the GitHub Actions workflow run

Powered by Super-linter

TRIVY

Report Summary

┌─────────┬──────┬─────────────────┬───────────────────┬─────────┐
│ Target  │ Type │ Vulnerabilities │ Misconfigurations │ Secrets │
├─────────┼──────┼─────────────────┼───────────────────┼─────────┤
│ pom.xml │ pom  │        1        │         -         │    -    │
└─────────┴──────┴─────────────────┴───────────────────┴─────────┘
Legend:
- '-': Not scanned
- '0': Clean (no security findings detected)


For OSS Maintainers: VEX Notice
--------------------------------
If you're an OSS maintainer and Trivy has detected vulnerabilities in your project that you believe are not actually exploitable, consider issuing a VEX (Vulnerability Exploitability eXchange) statement.
VEX allows you to communicate the actual status of vulnerabilities in your project, improving security transparency and reducing false positives for your users.
Learn more and start using VEX: https://trivy.dev/docs/v0.69/guide/supply-chain/vex/repo#publishing-vex-documents

To disable this notice, set the TRIVY_DISABLE_VEX_NOTICE environment variable.


pom.xml (pom)
=============
Total: 1 (UNKNOWN: 0, LOW: 0, MEDIUM: 0, HIGH: 1, CRITICAL: 0)

┌─────────────────────────────────────────┬─────────────────────┬──────────┬────────┬───────────────────┬───────────────────────┬────────────────────────────────────────────────────────┐
│                 Library                 │    Vulnerability    │ Severity │ Status │ Installed Version │     Fixed Version     │                         Title                          │
├─────────────────────────────────────────┼─────────────────────┼──────────┼────────┼───────────────────┼───────────────────────┼────────────────────────────────────────────────────────┤
│ com.fasterxml.jackson.core:jackson-core │ GHSA-72hv-8253-57qq │ HIGH     │ fixed  │ 2.21.0            │ 2.18.6, 2.21.1, 3.1.0 │ jackson-core: Number Length Constraint Bypass in Async │
│                                         │                     │          │        │                   │                       │ Parser Leads to Potential DoS...                       │
│                                         │                     │          │        │                   │                       │ https://github.com/advisories/GHSA-72hv-8253-57qq      │
└─────────────────────────────────────────┴─────────────────────┴──────────┴────────┴───────────────────┴───────────────────────┴────────────────────────────────────────────────────────┘

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 5, 2026

🤖 Claude Code Review

Here is my code review of this PR:


Code Review

Summary

This PR updates Java formatting/style tooling (Eclipse formatter XML, Checkstyle config), updates the macOS CI workflow and test script to use /opt/homebrew/lib, and unblocks previously suppressed Checkstyle rules.


Code Quality

❌ Hardcoded /opt/homebrew/lib path — potential breakage on Intel Macs

  • .github/workflows/maven-darwin.yaml line 50: DYLD_LIBRARY_PATH=${SENZING_PATH}/er/lib:/opt/homebrew/lib
  • src/test/java/com/senzing/test/GenerateTestJVMScript.java line 158: platformLibDir = new File("/opt/homebrew/lib");

/opt/homebrew is the Homebrew prefix on Apple Silicon Macs only. On Intel Macs, Homebrew installs to /usr/local, so /opt/homebrew/lib will not exist. The original path (${SENZING_PATH}/er/lib/macos) was relative to the Senzing installation. The replacement hardcodes an assumption about the machine architecture and Homebrew installation. Consider using $(brew --prefix)/lib dynamically in the workflow, and similarly in the Java test generator.

✅ No commented-out code

✅ Meaningful variable names

✅ DRY principle followed

⚠️ Potential Checkstyle/Formatter alignment concern

checkstyle.xml configures RightCurly with option=alone, which requires } to be on its own line (Allman style). The Eclipse formatter sets brace_position_for_block to next_line (also Allman), and insert_new_line_before_else_in_if_statement to insert. These appear consistent. However, the formatter's keep_then_statement_on_same_line = true could produce single-line if bodies that Checkstyle's NeedBraces (with allowSingleLineStatement=true) would accept — this combination deserves validation in practice to ensure no rule conflicts during the build.

❌ CLAUDE.md contains branch/session-specific content

Per the review checklist, .claude/CLAUDE.md should not contain anything specific to a local development environment. The ## Current Development Status section (line 252–258) contains:

Working on branch: `112-caceres-v4-upgrade-1`
- Modified files include URI classes and SQL client classes
- Test file renamed: `PostgreSqlUrlTest.java` → `PostgreSqlUriTest.java`

This is session/branch-specific state, not general project guidance. It should be removed from CLAUDE.md.


Testing

✅ No new functional logic introduced requiring unit tests — changes are tooling/config only, except for the GenerateTestJVMScript.java path change.

⚠️ GenerateTestJVMScript.java path change is not tested for Intel Mac compatibility — see the hardcoded path issue above.


Documentation

❌ No CHANGELOG.md update

CHANGELOG.md exists at the repo root but was not updated in this PR. Style tooling changes and CI updates warrant at least a brief entry.

✅ No README changes needed — the new checkstyle.xml and formatter config are self-contained tooling files.

✅ No API documentation impact


Security

✅ No hardcoded credentials

✅ No sensitive data in logs

✅ No license files (.lic) present

✅ No input validation concerns — changes are configuration only


Summary of Issues

Severity File Issue
❌ High .github/workflows/maven-darwin.yaml:50, GenerateTestJVMScript.java:158 Hardcoded /opt/homebrew/lib breaks Intel Mac builds
❌ Medium .claude/CLAUDE.md:252–258 Branch-specific state in project memory config
❌ Minor CHANGELOG.md Not updated for this PR
⚠️ Minor checkstyle.xml + .vscode/java-formatter.xml Formatter/Checkstyle interaction should be validated end-to-end before merge

Automated code review analyzing defects and coding standards

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 5, 2026

Super-linter summary

Language Validation result
CHECKOV Pass ✅
GITHUB_ACTIONS Pass ✅
GITHUB_ACTIONS_ZIZMOR Pass ✅
GITLEAKS Pass ✅
GIT_MERGE_CONFLICT_MARKERS Pass ✅
JSCPD Pass ✅
PRE_COMMIT Pass ✅
SPELL_CODESPELL Pass ✅
TRIVY Fail ❌
YAML Pass ✅
YAML_PRETTIER Pass ✅

Super-linter detected linting errors

For more information, see the GitHub Actions workflow run

Powered by Super-linter

TRIVY

Report Summary

┌─────────┬──────┬─────────────────┬───────────────────┬─────────┐
│ Target  │ Type │ Vulnerabilities │ Misconfigurations │ Secrets │
├─────────┼──────┼─────────────────┼───────────────────┼─────────┤
│ pom.xml │ pom  │        1        │         -         │    -    │
└─────────┴──────┴─────────────────┴───────────────────┴─────────┘
Legend:
- '-': Not scanned
- '0': Clean (no security findings detected)


For OSS Maintainers: VEX Notice
--------------------------------
If you're an OSS maintainer and Trivy has detected vulnerabilities in your project that you believe are not actually exploitable, consider issuing a VEX (Vulnerability Exploitability eXchange) statement.
VEX allows you to communicate the actual status of vulnerabilities in your project, improving security transparency and reducing false positives for your users.
Learn more and start using VEX: https://trivy.dev/docs/v0.69/guide/supply-chain/vex/repo#publishing-vex-documents

To disable this notice, set the TRIVY_DISABLE_VEX_NOTICE environment variable.


pom.xml (pom)
=============
Total: 1 (UNKNOWN: 0, LOW: 0, MEDIUM: 0, HIGH: 1, CRITICAL: 0)

┌─────────────────────────────────────────┬─────────────────────┬──────────┬────────┬───────────────────┬───────────────────────┬────────────────────────────────────────────────────────┐
│                 Library                 │    Vulnerability    │ Severity │ Status │ Installed Version │     Fixed Version     │                         Title                          │
├─────────────────────────────────────────┼─────────────────────┼──────────┼────────┼───────────────────┼───────────────────────┼────────────────────────────────────────────────────────┤
│ com.fasterxml.jackson.core:jackson-core │ GHSA-72hv-8253-57qq │ HIGH     │ fixed  │ 2.21.0            │ 2.18.6, 2.21.1, 3.1.0 │ jackson-core: Number Length Constraint Bypass in Async │
│                                         │                     │          │        │                   │                       │ Parser Leads to Potential DoS...                       │
│                                         │                     │          │        │                   │                       │ https://github.com/advisories/GHSA-72hv-8253-57qq      │
└─────────────────────────────────────────┴─────────────────────┴──────────┴────────┴───────────────────┴───────────────────────┴────────────────────────────────────────────────────────┘

…ms and updated dependencies for version 2.0.0-beta.2.2

Fixed SQLiteUri.parse() to use direct File construction instead of java.net.URI
which rejected unencoded spaces (e.g. macOS ~/Library/Application Support/).
Fixed SQLiteUri.toString() to emit raw file paths without URL encoding since
the Senzing engine treats paths literally. Added path validation for non-Windows
paths to reject backslashes and require absolute or relative path prefixes.
Updated dependencies including jackson-bom, armeria-bom, netty-bom, sqlite-jdbc,
sqs, amqp-client, embedded-postgres, and Maven plugins.
@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review

Now I have enough context for a thorough review.


PR Code Review: 2.0.0-beta.2.2

Code Quality

✅ Code style — The changes follow the project's established conventions. The new checkstyle.xml and formatter XML enforce Allman-style braces and 4-space indentation consistently with the existing codebase. Removing the LeftCurly and LineLength suppressions from checkstyle-suppressions.xml is a positive cleanup.

✅ No commented-out code — All comments added are explanatory, not dead code.

✅ Meaningful variable names — The rename of urluri in SQLiteUri.equals() (line 404) is a good consistency fix.

✅ DRY principle — Both path branches in parse() now call new File(urlDecodeUtf8(path)). This is acceptable since the logic diverges for validation, not for the final construction step.

❌ Defect — CHANGELOG version mismatches (CHANGELOG.md lines 19–28):

Two dependency versions listed in the CHANGELOG do not match the actual pom.xml diffs:

  • sqlite-jdbc: CHANGELOG says 3.51.2.0 → 3.51.3.0, but pom.xml shows 3.51.1.0 → 3.51.3.0
  • sqs: CHANGELOG says 2.41.28 → 2.42.18, but pom.xml shows 2.41.25 → 2.42.18

Additionally, the postgresql update (42.7.9 → 42.7.10) is visible in pom.xml but not mentioned in the CHANGELOG.

⚠️ Potential edge case — % in file paths (SQLiteUri.java line 365 / 525):

toString() now emits raw paths (unencoded), but parse() always calls urlDecodeUtf8(path) on the non-Windows branch. If a file path legitimately contains a % character (e.g. /tmp/test%data/file.db), the round-trip would corrupt the path: toString() emits /tmp/test%data/file.db, then parse() attempts to URL-decode %da as a byte sequence. This is an uncommon but valid file path scenario on Unix.

⚠️ Hardcoded Homebrew path (GenerateTestJVMScript.java line 158, maven-darwin.yaml line 50):

/opt/homebrew/lib is hardcoded for macOS. The comment documents that Intel Macs (/usr/local/lib) are intentionally dropped. This is acceptable given the ARM-only decision, but it should be explicitly noted in the PR description or README if not already done, as it is a breaking change for any Intel Mac developers.


Testing

✅ New constructor test cases added for paths with spaces (SQLiteUriTest.java lines 87–89).

✅ New negative test case for relative paths (sqlite3://relative/path.db) added to testBadUriParse.

❌ Missing parse() round-trip tests for space-containing paths — There are no explicit tests verifying that SQLiteUri.parse("sqlite3:///path%20with%20spaces/file.db") and SQLiteUri.parse("sqlite3:///path with spaces/file.db") produce the correct File object and round-trip correctly through toString(). The fix is specifically for parse() behavior, so direct parse-path tests would strengthen confidence.

❌ No test for the % in filename edge case described above.


Documentation

✅ CHANGELOG updated with the correct version and meaningful descriptions of changes.

⚠️ CHANGELOG inaccuracies as noted above (wrong prior versions for sqlite-jdbc and sqs; missing postgresql bump).

.claude/CLAUDE.md updated — Branch-specific, session-local context removed. The remaining entries are general and appropriate for any developer.

✅ Inline comments are clear and explain the non-obvious decision not to URL-encode on non-Windows paths.


Security

✅ No hardcoded credentials.

✅ No .lic files checked in.

✅ The unusedPassword comment (SQLiteUri.java line 227–229) correctly clarifies why logging that value is acceptable. This is good practice.

✅ Input validation improved — The new validation in parse() rejects backslashes and bare relative paths on non-Windows, which tightens the API contract.


Summary

Area Status
Code style
No commented-out code
Meaningful names
DRY
Defects / edge cases ❌ CHANGELOG version mismatches; % in path round-trip risk
Tests ❌ Missing direct parse() tests for space paths
Documentation ⚠️ CHANGELOG has wrong prior versions for 2 deps; missing postgresql bump
Security

Blocking issues before merge:

  1. Fix the three CHANGELOG inaccuracies (wrong prior versions for sqlite-jdbc and sqs; add the postgresql update entry).
  2. Consider adding parse() round-trip tests for paths with spaces (both encoded and unencoded forms).

Non-blocking suggestions:

  • Address the % in filename edge case in parse() (either document it as unsupported or add a guard).

Automated code review analyzing defects and coding standards

@github-actions
Copy link
Copy Markdown

Super-linter summary

Language Validation result
CHECKOV Pass ✅
GITHUB_ACTIONS Pass ✅
GITHUB_ACTIONS_ZIZMOR Pass ✅
GITLEAKS Pass ✅
GIT_MERGE_CONFLICT_MARKERS Pass ✅
JSCPD Pass ✅
PRE_COMMIT Pass ✅
SPELL_CODESPELL Pass ✅
TRIVY Pass ✅
YAML Pass ✅
YAML_PRETTIER Pass ✅

All files and directories linted successfully

For more information, see the GitHub Actions workflow run

Powered by Super-linter

…issing entries

Added comment in maven-darwin.yaml noting /opt/homebrew/lib is ARM64-only.
Updated CHANGELOG.md to include the macOS CI workflow library path change
and the Java code style/formatting configuration updates.
@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review

PR Code Review: 2.0.0-beta.2.2


Code Quality

✅ No commented-out code
New comments are explanatory (not commented-out code).

✅ Meaningful variable names
Rename from urluri in SQLiteUri.equals() at SQLiteUri.java:401-408 is an improvement.

✅ DRY principle followed
No obvious duplication introduced.

⚠️ Style guide — Unable to fetch the style guide URL (network not available in this environment). Review against it manually.

✅ Defects / Logic Analysis

  • SQLiteUri.parse() fix looks correct: removes the fragile java.net.URI path that rejected unencoded spaces, replaces with direct File(urlDecodeUtf8(path)). This correctly handles both encoded and unencoded spaces.
  • SQLiteUri.toString() fix correctly uses raw file.getPath() instead of URL-encoding tokens with urlEncodeUtf8() which was using + for spaces (form-encoding, not path encoding).
  • Path validation in parse() correctly rejects backslashes and bare relative paths (e.g., sqlite3://relative/path.db) on non-Windows.
  • Minor concern: The comment at SQLiteUri.java:228-230 says logging unusedPassword "is fine" because it is not a real credential. However, the password is still being interpolated into the exception message — this is technically logging a field named "password." Even if it's a placeholder, it establishes a pattern that could be problematic if the field semantics change. Consider redacting it or removing the comment justification.

✅ No security vulnerabilities introduced

✅ CLAUDE.md update
The update removes branch-specific state (112-caceres-v4-upgrade-1) and replaces it with stable, general-purpose information. This is appropriate — the file is now suitable for any developer.


Testing

✅ Unit tests for new paths with spaces
SQLiteUriTest.java adds:

  • Constructor test with /path with spaces/file.db
  • Constructor test with /Users/test/Library/Application Support/test.db
  • Bad-URI test for sqlite3://relative/path.db (validates new rejection logic)

⚠️ Missing round-trip test for spaces in toString()
The bug fix for toString() (no URL-encoding of spaces) should have a dedicated test verifying that SQLiteUri.parse(uri).toString() produces the expected raw path string for inputs containing spaces. The constructor tests verify construction, but the toString() regression is not explicitly covered.

⚠️ Missing test: parse() with URL-encoded space (%20) in path
The new code path calls urlDecodeUtf8(path) so sqlite3:///path%20with%20spaces/file.db should work. No test added for that case.


Documentation

✅ CHANGELOG.md updated — Version 2.0.0-beta.2.2 entry is detailed and accurate.

✅ Trailing whitespace fix in CHANGELOG.md
CHANGELOG.md:85 — trailing space after URI's. removed. Good cleanup.

✅ Inline comments for complex logic
Comments in SQLiteUri.java around the non-Windows path handling are clear and useful.

⚠️ CHANGELOG version discrepancy
CHANGELOG.md entry mentions:

Updated sqlite-jdbc from version 3.51.2.0 to 3.51.3.0

But pom.xml diff shows the change is from 3.51.1.03.51.3.0. The 3.51.2.0 intermediate version mentioned in the changelog is not reflected anywhere.

Also CHANGELOG mentions:

Updated Amazon sqs from version 2.41.28 to 2.42.18

But pom.xml diff shows the change is from 2.41.252.42.18. The 2.41.28 starting version in the CHANGELOG is incorrect.

These should be corrected to match the actual pom.xml changes.


Security

✅ No hardcoded credentials

✅ No license files (.lic) with AQAAAD

⚠️ Password in exception message (SQLiteUri.java:233)

throw new IllegalArgumentException("Inconsistent user credentials.  Either both or neither "
    + "should be null.  unusedUser=[ " + unusedUser + " ], unusedPassword=[ " + unusedPassword + " ]");

The comment added in this PR explicitly justifies logging the password field. Even though it's documented as a placeholder, embedding a field named unusedPassword in exception messages (which often surface in logs) is a security smell. Recommend redacting: unusedPassword=[ [REDACTED] ].


Summary of Issues

Severity Issue
Medium CHANGELOG dependency version numbers are wrong (sqlite-jdbc and sqs starting versions don't match pom.xml)
⚠️ Low Password field interpolated into exception message with justifying comment — recommend redacting
⚠️ Low Missing toString() round-trip test for paths with spaces
⚠️ Low Missing parse() test with URL-encoded spaces (%20)

The core bug fixes are sound and the logic is correct. The main actionable items are the CHANGELOG version inaccuracies and the optional test coverage gaps.

Automated code review analyzing defects and coding standards

@github-actions
Copy link
Copy Markdown

Super-linter summary

Language Validation result
CHECKOV Pass ✅
GITHUB_ACTIONS Pass ✅
GITHUB_ACTIONS_ZIZMOR Pass ✅
GITLEAKS Pass ✅
GIT_MERGE_CONFLICT_MARKERS Pass ✅
JSCPD Pass ✅
PRE_COMMIT Pass ✅
SPELL_CODESPELL Pass ✅
TRIVY Pass ✅
YAML Pass ✅
YAML_PRETTIER Pass ✅

All files and directories linted successfully

For more information, see the GitHub Actions workflow run

Powered by Super-linter

…aces

Corrected CHANGELOG "from" versions for sqlite-jdbc (3.51.1.0) and sqs
(2.41.22) to match the 2.0.0-beta.2.1 release. Added missing postgresql
42.7.9 to 42.7.10 update entry. Added testParseEncodedSpaces test to
verify parse() decodes %20 in paths and toString() emits raw unencoded paths.
@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review

PR Code Review

Code Quality

Code style guide - The changes follow Java conventions with proper indentation, spacing, and formatting consistent with the project's style.

No commented-out code - Comments added are explanatory (e.g., // NOTE: logging unusedPassword here is fine...), not disabled code.

Meaningful variable names - url renamed to uri in SQLiteUri.java:404 is an improvement.

DRY principle - The duplicate URL-decode path in SQLiteUri.parse() at src/main/java/com/senzing/datamart/SQLiteUri.java:527 is acceptable — both branches now use urlDecodeUtf8(path) but with distinct validation logic.

⚠️ Potential defect — CHANGELOG version mismatch: CHANGELOG.md line ~35 says sqs was updated from 2.41.22 to 2.42.18, but pom.xml shows the old version was 2.41.25, not 2.41.22. The CHANGELOG entry is inaccurate.

⚠️ Potential defect — sqlite-jdbc version mismatch: CHANGELOG.md says updated from 3.51.1.0 to 3.51.3.0, but pom.xml diff shows old version 3.51.1.03.51.3.0. The CHANGELOG says 3.50.3.0 in the Dependencies section of CLAUDE.md — minor inconsistency but not a blocker.

No bugs/logic errors - The SQLiteUri fixes are logically sound: removing java.net.URI parsing avoids URISyntaxException for unencoded spaces, and using raw paths for toString() is correct for Senzing engine consumption.

CLAUDE.md is general-purpose - The branch-specific lines (Working on branch: 112-caceres-v4-upgrade-1) were correctly removed. The remaining content is appropriate for any developer.


Testing

Unit tests for new functions - New testParseEncodedSpaces parameterized test covers %20-encoded paths and round-trip toString() verification.

New bad-URI test case - "sqlite3://relative/path.db" added to testBadUriParse to cover the new relative-path rejection.

Constructor test cases - Paths with spaces added in getConstructParameters() at SQLiteUriTest.java:87-89.

⚠️ Missing edge case - The new validation rejects paths not starting with /, ./, or ../. There is no test for a path starting with ../ (explicit relative path) to confirm it is accepted. Consider adding a test case like sqlite3://../relative/path.db to verify the ../ branch.

⚠️ Missing negative test - No test for a non-Windows path containing a backslash (e.g., sqlite3:///path\\with\\backslash/file.db) to exercise the new backslash rejection branch.


Documentation

CHANGELOG.md updated - Version 2.0.0-beta.2.2 entry is comprehensive and well-structured.

Inline comments for complex logic - The NOTE: comments in SQLiteUri.java and GenerateTestJVMScript.java appropriately explain non-obvious decisions (password logging safety, ARM64-only support).

README not required - No public API or user-facing interface changes warrant README updates.

⚠️ CHANGELOG trailing whitespace - The diff at CHANGELOG.md:87 shows a trailing space was removed ("URI's. This was done ""URI's. This was done"). Good fix, but verify no other trailing whitespace was introduced in the new entry.

⚠️ Markdown formatting - The CHANGELOG entry uses inconsistent line wrapping (some lines exceed 80 chars). While not a strict violation, prettier would flag some lines in the dependency list. Example: " - Updated Amazon \sqs` from version `2.41.22` to `2.42.18`"`.


Security

No hardcoded credentials - The unusedPassword comment correctly notes it is a non-secret placeholder value ("na"), not a real credential.

Input validation - New path validation in SQLiteUri.parse() properly rejects backslashes and non-absolute/non-relative paths.

No sensitive data in logs - The error messages in SQLiteUri.java:509,516 include the path and URI, which is acceptable for diagnostic purposes (no passwords logged).

No hardcoded credentials or license files - No .lic files or AQAAAD-prefixed strings found.

Proper error handling - IllegalArgumentException thrown with descriptive messages for invalid inputs.


Summary

Category Status
Code Quality ✅ with minor CHANGELOG version mismatch
Testing ⚠️ Missing edge cases for ../ and backslash rejection
Documentation ⚠️ CHANGELOG SQS version number inaccuracy
Security

Recommended fixes before merge:

  1. CHANGELOG.md: Fix the SQS "from" version — should be 2.41.25, not 2.41.22 (matches pom.xml diff).
  2. SQLiteUriTest.java: Add a test case for backslash rejection on non-Windows paths to testBadUriParse.
  3. SQLiteUriTest.java: Consider adding a ../-prefixed relative path to testParseEncodedSpaces or a separate test to confirm the ../ branch is accepted.

Automated code review analyzing defects and coding standards

@github-actions
Copy link
Copy Markdown

Super-linter summary

Language Validation result
CHECKOV Pass ✅
GITHUB_ACTIONS Pass ✅
GITHUB_ACTIONS_ZIZMOR Pass ✅
GITLEAKS Pass ✅
GIT_MERGE_CONFLICT_MARKERS Pass ✅
JSCPD Pass ✅
PRE_COMMIT Pass ✅
SPELL_CODESPELL Pass ✅
TRIVY Pass ✅
YAML Pass ✅
YAML_PRETTIER Pass ✅

All files and directories linted successfully

For more information, see the GitHub Actions workflow run

Powered by Super-linter

@barrycaceres barrycaceres enabled auto-merge (squash) March 23, 2026 20:42
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 23, 2026

Code Coverage

Overall Project 93.69% 🟢
Files changed 100% 🟢

File Coverage
SQLiteUri.java 96.68% 🟢

On Windows, java.io.File converts forward slashes to backslashes, causing
Unix-style path test parameters to produce backslash paths that fail parsing
and assertion checks. Wrapped Unix path test parameters in a platform check
and added @DisabledOnOs(OS.WINDOWS) to testParseEncodedSpaces.
@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review


Code Review: PR to main (2.0.0-beta.2.2)

Code Quality

Code style — The code changes follow consistent formatting (4-space indentation, Allman-style braces). The new checkstyle.xml and java-formatter.xml enforce these conventions. The suppression removals in checkstyle-suppressions.xml align with the newly active rules.

No commented-out code — All comments are explanatory, not dead code.

Meaningful variable names — The rename of urluri in SQLiteUri.java:401 (equals()) is a minor but correct improvement.

DRY — The path-handling simplification in SQLiteUri reduces duplication in toString() and parse().

⚠️ Misleading commentSQLiteUri.java:228:

// NOTE: logging unusedPassword here is fine — it is not a real credential...

The word "logging" is inaccurate — the code is throwing an exception that embeds unusedPassword in its message, not logging. While the comment correctly explains why it's safe, a future developer might wonder why "logging" applies to an exception. Suggest: change "logging" to "including in the exception message."

No defects found in logic — The parse() / toString() symmetry for non-Windows paths with spaces appears correct. The GenerateTestJVMScript.java change is consistent with the workflow: libDir is still {senzingDir}/lib (line 157), and platformLibDir is separately set to /opt/homebrew/lib (line 160). The resulting library path (libDir:platformLibDir) matches ${SENZING_PATH}/er/lib:/opt/homebrew/lib in the workflow (line 50 of maven-darwin.yaml). ✅

CLAUDE.md — The update correctly strips branch-specific state and replaces it with permanent project facts. Suitable for any developer on any machine.


Testing

New tests addedtestParseEncodedSpaces covers the bug fix for URL-encoded spaces in non-Windows paths, including both parse() and toString() assertions.

Platform guard@DisabledOnOs(OS.WINDOWS) is correctly used on the new tests and in getConstructParameters().

New negative test"sqlite3://relative/path.db" added to testBadUriParse to cover the new relative-path validation.

⚠️ Missing test case for backslash rejection — The new parse() code rejects backslashes in non-Windows paths (SQLiteUri.java:509-512), but there is no test asserting that a path like sqlite3:///path\\with\\backslashes/file.db throws on non-Windows. The existing testBadUriParse entry "sqlite3://foo\\bar\\phoo" may cover this partially, but it's a different path structure. Consider adding an explicit case.


Documentation

CHANGELOG.md updated — The new [2.0.0-beta.2.2] section is detailed and accurate.

CHANGELOG version inaccuracyCHANGELOG.md (new entry, dependency list) states:

Updated Amazon sqs from version 2.41.22 to 2.42.18

But pom.xml shows the version going from 2.41.252.42.18. The "from" version is wrong. File: CHANGELOG.md, in the 2.0.0-beta.2.2 section.

⚠️ netty-bom not visible in pom.xml — CHANGELOG mentions updating netty-bom from 4.2.9.Final to 4.2.10.Final, but this change is not in the pom.xml diff. If it's managed transitively by armeria-bom, that's fine, but the CHANGELOG entry could mislead readers into thinking there's a direct netty-bom version property in the POM.

Trailing whitespace fixed — The old CHANGELOG.md entry for 2.0.0-beta.1.1 had trailing whitespace; that is fixed.

No README changes needed — This is a bug-fix/tooling/dependency release.


Security

No hardcoded credentials — The unusedPassword in the exception message is documented as a Senzing-specific non-credential placeholder ("na"), not a real secret.

No .lic files or AQAAAD-prefixed content found in the diff.

No sensitive data in logs — Exception messages only include structural path information.

Input validation — The new path validation in SQLiteUri.parse() (SQLiteUri.java:509-522) properly rejects backslashes and ambiguous relative paths on non-Windows.


Summary

Category Status
Code style ✅ Pass
No commented-out code ✅ Pass
Meaningful names ✅ Pass
DRY ✅ Pass
Defects ✅ None found
CLAUDE.md appropriateness ✅ Pass
Tests for new code ✅ Pass
Backslash-rejection test ⚠️ Missing
CHANGELOG updated ✅ Pass
CHANGELOG SQS version ❌ Incorrect (2.41.22 should be 2.41.25)
netty-bom in pom.xml ⚠️ Not visible in diff
Security ✅ Pass

Required fix: Correct the SQS "from" version in CHANGELOG.md from 2.41.22 to 2.41.25.

Recommended: Fix the misleading "logging" word in the SQLiteUri.java:228 comment, and add a test case for backslash rejection on non-Windows.

Automated code review analyzing defects and coding standards

@github-actions
Copy link
Copy Markdown

Super-linter summary

Language Validation result
CHECKOV Pass ✅
GITHUB_ACTIONS Pass ✅
GITHUB_ACTIONS_ZIZMOR Pass ✅
GITLEAKS Pass ✅
GIT_MERGE_CONFLICT_MARKERS Pass ✅
JSCPD Pass ✅
PRE_COMMIT Pass ✅
SPELL_CODESPELL Pass ✅
TRIVY Pass ✅
YAML Pass ✅
YAML_PRETTIER Pass ✅

All files and directories linted successfully

For more information, see the GitHub Actions workflow run

Powered by Super-linter

@barrycaceres barrycaceres merged commit c90b61d into main Mar 23, 2026
39 checks passed
@barrycaceres barrycaceres deleted the caceres-homebrew-libs-1 branch March 23, 2026 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants