#757 Add support for transparent GPG/PGP decryption of files on read - #868
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCobrix now accepts optional ASCII-armored GPG credentials through reader options. It decrypts GPG input streams before compression handling and propagates credentials through Spark processing, indexing, and scanning. The change adds Bouncy Castle support, documentation, fixtures, and tests. ChangesTransparent PGP decryption
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SparkReader
participant FileStreamer
participant BufferedFSDataInputStream
participant GpgUtils
participant BouncyCastle
SparkReader->>FileStreamer: open input with GPG credentials
FileStreamer->>BufferedFSDataInputStream: create stream
BufferedFSDataInputStream->>GpgUtils: decrypt encrypted input
GpgUtils->>BouncyCastle: parse key ring and decrypt message
BouncyCastle-->>GpgUtils: provide literal data stream
GpgUtils-->>BufferedFSDataInputStream: return decrypted stream
BufferedFSDataInputStream-->>SparkReader: provide readable data
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/scanners/CobolScanners.scala (1)
75-84: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftCalculate GPG ranges from decrypted bytes.
When
gpgPrivateKeyis set andfileEndOffsetis non-zero,FileStatus.getLenreturns the encrypted file length.FileStreamerappliesmaximumFileBytesafter decryption. Ciphertext and plaintext lengths differ, so this path can scan the wrong record range.Calculate the limit from the decrypted stream length. Otherwise, reject
file_end_offsetfor GPG input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/scanners/CobolScanners.scala` around lines 75 - 84, Update the file-size calculation in the scanner flow before constructing the two FileStreamer instances so GPG input derives maximumFileBytes from the decrypted stream length, not FileStatus.getLen; when a decrypted length cannot be obtained, reject non-zero fileEndOffset for GPG input instead of scanning an incorrect range.
🧹 Nitpick comments (2)
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test45PgpEncryptedFilesSpec.scala (1)
39-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd an encrypted variable-length indexed-read test.
This suite tests fixed-record input and disables indexes in its second case. It does not execute the changed GPG paths in
IndexBuilderandCobolScanners.Add an encrypted variable-length fixture with indexes enabled. Assert that indexed and non-indexed reads return the same rows.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test45PgpEncryptedFilesSpec.scala` around lines 39 - 79, Add a third test under the “gpg encrypted files” suite that reads an encrypted variable-length fixture with indexes enabled, then reads the same fixture with indexes disabled and asserts both results contain identical rows. Reuse the existing key-loading and Spark read configuration patterns, and add or reference the corresponding variable-length fixture and expected data needed by the test.cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParameters.scala (1)
89-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGPG fields are inserted mid-signature in two public case classes. Both classes belong to the published
cobol-parserartifact, and both place the new fields afterisUtf16BigEndian. Positional construction by downstream code breaks in both cases. The shared fix is to append the fields at the end of each parameter list.
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParameters.scala#L89-L90: movegpgPrivateKeyandgpgPrivateKeyPassphraseto the end of the parameter list and give them= Nonedefaults, then update the named arguments inCobolParametersParser.parse.cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/ReaderParameters.scala#L100-L101: move the two fields to the end of the parameter list, keeping the existing= Nonedefaults.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParameters.scala` around lines 89 - 90, Preserve positional-construction compatibility by moving gpgPrivateKey and gpgPrivateKeyPassphrase to the end of both public case-class parameter lists: in cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParameters.scala:89-90, add = None defaults and update named arguments in CobolParametersParser.parse; in cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/ReaderParameters.scala:100-101, move the fields while retaining their existing defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spark-cobol/pom.xml`:
- Around line 52-57: Add an org.bouncycastle.** relocation rule to the root
POM’s maven-shade-plugin configuration, alongside the existing execution
settings, matching the relocation already defined in build.sbt. Ensure the
shaded spark-cobol artifact rewrites all BouncyCastle packages without changing
the dependency declaration.
In
`@spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/BufferedFSDataInputStream.scala`:
- Around line 131-146: Retain the raw FSDataInputStream opened in the GPG branch
of BufferedFSDataInputStream, including when GpgUtils.decryptStream throws, and
update close() to close that raw stream after closing the decrypted input stream
when it is non-null. Keep the existing compressed and uncompressed stream
handling unchanged.
- Around line 148-150: Update the compressed/non-seekable branch in
BufferedFSDataInputStream’s start-offset handling to advance by repeatedly
reading into a scratch buffer, rather than calling baseStream.skip. Continue
until the requested offset is consumed or read() returns -1, ensuring zero-byte
skip behavior cannot prematurely stop FileStreamer.next with fewer bytes.
In
`@spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/utils/GpgUtils.scala`:
- Around line 81-87: Update GpgUtils.scala in the bcProvider initialization to
reuse the registered provider only when it is an instance of the relocated
BouncyCastleProvider; otherwise create and use a private unregistered instance.
Keep build.sbt lines 153-154 unchanged, as the existing relocation rule requires
no direct modification.
---
Outside diff comments:
In
`@spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/scanners/CobolScanners.scala`:
- Around line 75-84: Update the file-size calculation in the scanner flow before
constructing the two FileStreamer instances so GPG input derives
maximumFileBytes from the decrypted stream length, not FileStatus.getLen; when a
decrypted length cannot be obtained, reject non-zero fileEndOffset for GPG input
instead of scanning an incorrect range.
---
Nitpick comments:
In
`@cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParameters.scala`:
- Around line 89-90: Preserve positional-construction compatibility by moving
gpgPrivateKey and gpgPrivateKeyPassphrase to the end of both public case-class
parameter lists: in
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParameters.scala:89-90,
add = None defaults and update named arguments in CobolParametersParser.parse;
in
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/ReaderParameters.scala:100-101,
move the fields while retaining their existing defaults.
In
`@spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test45PgpEncryptedFilesSpec.scala`:
- Around line 39-79: Add a third test under the “gpg encrypted files” suite that
reads an encrypted variable-length fixture with indexes enabled, then reads the
same fixture with indexes disabled and asserts both results contain identical
rows. Reuse the existing key-loading and Spark read configuration patterns, and
add or reference the corresponding variable-length fixture and expected data
needed by the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6bf65c85-d123-4818-abc1-a1174c6aea38
📒 Files selected for processing (18)
README.mdbuild.sbtcobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParameters.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParametersParser.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/ReaderParameters.scalapom.xmlproject/Dependencies.scalaspark-cobol/pom.xmlspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/SparkCobolProcessor.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/DefaultSource.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/index/IndexBuilder.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/scanners/CobolScanners.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/BufferedFSDataInputStream.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/FileStreamer.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/utils/GpgUtils.scalaspark-cobol/src/test/resources/test/test_gpg_key.ascspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test45PgpEncryptedFilesSpec.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/utils/GpgUtilsSuite.scala
Comment on lines
+52
to
+57
| <!-- GPG support --> | ||
| <dependency> | ||
| <groupId>org.bouncycastle</groupId> | ||
| <artifactId>bcpg-jdk18on</artifactId> | ||
| </dependency> | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect shade/relocation configuration in both POMs.
fd -t f 'pom.xml' | while IFS= read -r f; do
echo "=== $f ==="
rg -n -C8 'maven-shade-plugin|<relocation>|<relocations>|shadedPattern|bouncycastle' "$f"
doneRepository: AbsaOSS/cobrix
Length of output: 3790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== root pom relevant sections ==="
sed -n '90,220p' pom.xml
sed -n '350,520p' pom.xml
echo "=== spark-cobol pom relevant sections ==="
sed -n '1,120p' spark-cobol/pom.xml
echo "=== build.sbt relevant shading lines ==="
sed -n '140,165p' build.sbt
echo "=== all occurrences of bouncycastle relocate-like references ==="
rg -n -i 'relocate|relocations|bouncycastle|bcpg-jdk18on|bcprov' . -g '!target' -g '!\.git' -g '!out'Repository: AbsaOSS/cobrix
Length of output: 13347
Add BouncyCastle relocation to the Maven shade configuration.
build.sbt relocates org.bouncycastle.**, but the root POM’s maven-shade-plugin configuration only has <execution> and no <relocations>. Add the same BouncyCastle relocation for Maven so the released spark-cobol jar does not expose BouncyCastle classes that can collide with the cluster classpath.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spark-cobol/pom.xml` around lines 52 - 57, Add an org.bouncycastle.**
relocation rule to the root POM’s maven-shade-plugin configuration, alongside
the existing execution settings, matching the relocation already defined in
build.sbt. Ensure the shaded spark-cobol artifact rewrites all BouncyCastle
packages without changing the dependency declaration.
Comment on lines
+148
to
+150
| if (startOffset > 0) { | ||
| if (codec == null) { | ||
| fsIn.seek(startOffset) | ||
| if (!isCompressedStream) { | ||
| baseStream.asInstanceOf[FSDataInputStream].seek(startOffset) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate all callers that construct streams with a non-zero start offset for encrypted input.
rg -nP --type=scala -C4 'new FileStreamer\('
rg -nP --type=scala -C4 'def getStreams\('Repository: AbsaOSS/cobrix
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -u
echo "== find file =="
fd -a 'BufferedFSDataInputStream\.scala$' . || true
echo "== git status/diff stat =="
git status --short || true
git diff --stat || true
echo "== search for BufferFS / FileStreamer / startOffset / isCompressedStream =="
rg -n -C 3 'BufferedFSDataInputStream|FileStreamer|startOffset|isCompressedStream|skip(' . -S || trueRepository: AbsaOSS/cobrix
Length of output: 531
🏁 Script executed:
#!/bin/bash
set -u
file="$(fd 'BufferedFSDataInputStream\.scala$' . | head -n 1 || true)"
if [ -n "$file" ]; then
echo "== line count and outline =="
wc -l "$file"
ast-grep outline "$file" || true
echo "== relevant lines 1-240 =="
sed -n '1,240p' "$file" | nl -ba | sed -n '1,240p'
fiRepository: AbsaOSS/cobrix
Length of output: 479
🏁 Script executed:
#!/bin/bash
set -u
file="$(fd 'BufferedFSDataInputStream\.scala$' . | head -n 1)"
echo "== file =="
printf '%s\n' "$file"
echo "== lines =="
awk '{printf "%5d\t%s\n", NR, $0}' "$file"
echo "== searches =="
rg -n -C 3 'BufferedFSDataInputStream|FileStreamer|startOffset|isCompressedStream|skip\(' . -S || trueRepository: AbsaOSS/cobrix
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
echo "== FileStreamer definitions/usages =="
rg -n -C 4 'class FileStreamer|object FileStreamer|new FileStreamer\s*\(|def FileStreamer\s*\(' spark-cobol cobol-parser -S
echo "== GpgUtils.decryptStream signature/usages =="
fd -a 'GpgUtils\.scala$' . | xargs -r sed -n '1,220p'
echo "== stream wrapper/read stream behavior around buffered stream =="
sed -n '1,280p' spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/SparkCobolProcessor.scala | awk '{printf "%5d\t%s\n", NR, $0}'
echo "== readFully/read stream consumer candidates =="
rg -n -C 3 'readFully\s*\(|next\r?\(|BufferedFSDataInputStream|readFullyHelper' spark-cobol/src/main/scala -SRepository: AbsaOSS/cobrix
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -u
echo "== FileStreamer.scala =="
awk '{printf "%5d\t%s\n", NR, $0}' spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/FileStreamer.scala
echo "== exact non-test non-deprecation non-log skip/readFully search =="
rg -n -C 4 'def\s+(next|\bread\b|readFully|available|close|size)|readFully\s*\(|next\s*\(|skip\s*\(|new\s+BufferedFSDataInputStream' spark-cobol/src/main/scala spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source -S
echo "== deterministic InputStream.skip contract probe from installed OpenJDK docs/source if available =="
java -version 2>&1 | head -5 || trueRepository: AbsaOSS/cobrix
Length of output: 47155
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
from pathlib import Path
p = Path('spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/BufferedFSDataInputStream.scala')
src = p.read_text()
skip_block = src[src.find('while (toSkip > 0)'):src.find('}')+1]
print('skip_block_contains_return_to_skip_zero:', return_to_skip_zero := 'if (skipped <= 0) return baseStream' in skip_block)
print('skip_block_contains_only_skip_loop:', all(s in skip_block for s in ('baseStream.skip(toSkip)', 'toSkip -= skipped')))
# Simulate the loop with a deterministic "skip returns 0 but stream remains".
def simulated_skip_stream(total):
skipped = 0
while skipped < total:
skipped0 = 0
if skipped0 <= 0:
return skipped
skipped += skipped0
return skipped
print('simulated_skip_with_return_zero_returns_skipped:', simulated_skip_stream(123))
print('simulated_skip_reading_until_target_returns_target', 'read scratch loop should return target if stream remains')
PY
echo "== Java InputStream.skip Javadoc on installed JDK, if available =="
find "$JAVA_HOME" "$JDK_HOME" /usr/lib/jvm -maxdepth 2 \( -name '*.html' -o -name '*.src' \) 2>/dev/null | xargs -r grep -l 'skip(long n)' | head -5 | while read -r f; do
printf '\nFILE: %s\n' "$f"
grep -n -C 3 'Returns:|skipped|InputStream.skip' "$f" | head -40
done | head -120 || trueRepository: AbsaOSS/cobrix
Length of output: 509
Skip by reading instead of relying on InputStream.skip.
In non-seekable compressed/PGP streams, baseStream.skip(toSkip) can return 0 while data remains; the loop then stops and FileStreamer.next can return fewer bytes than requested without an error. Replace this loop with a read-scratch approach and only stop on read() returning -1.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/BufferedFSDataInputStream.scala`
around lines 148 - 150, Update the compressed/non-seekable branch in
BufferedFSDataInputStream’s start-offset handling to advance by repeatedly
reading into a scratch buffer, rather than calling baseStream.skip. Continue
until the requested offset is consumed or read() returns -1, ensuring zero-byte
skip behavior cannot prematurely stop FileStreamer.next with fewer bytes.
…o enable transparent decryption of GPG-encrypted files.
yruslan
force-pushed
the
feature/757-gpg-decryption
branch
from
August 4, 2026 08:44
c26f765 to
64fa1d7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 1699-1707: Change the “File reading options” subsection heading in
the README from level five to level three, using “### File reading options”
beneath “## Summary of all available options” to maintain the required heading
hierarchy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 75d47797-54b7-4a27-96d3-a59994785c35
📒 Files selected for processing (18)
README.mdcobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParameters.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParametersParser.scalacobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/ReaderParameters.scaladata/test41_copybook.cobdata/test41_data/example.bin.gpgdata/test41_expected/test41a.txtdata/test41_expected/test41b.txtspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/SparkCobolProcessor.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/DefaultSource.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/index/IndexBuilder.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/scanners/CobolScanners.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/BufferedFSDataInputStream.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/FileStreamer.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/utils/GpgUtils.scalaspark-cobol/src/test/resources/test/test_gpg_file.gpgspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test45PgpEncryptedFilesSpec.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/utils/GpgUtilsSuite.scala
🚧 Files skipped from review as they are similar to previous changes (9)
- spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/SparkCobolProcessor.scala
- cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParameters.scala
- cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/CobolParametersParser.scala
- cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/reader/parameters/ReaderParameters.scala
- spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/utils/GpgUtils.scala
- spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/FileStreamer.scala
- spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/index/IndexBuilder.scala
- spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/DefaultSource.scala
- spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/scanners/CobolScanners.scala
Comment on lines
+1699
to
+1707
| | Option (usage example) | Description | | ||
| |-----------------------------------------------------|:------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | .option("data_paths", "/path1,/path2") | Allows loading data from multiple unrelated paths on the same filesystem. | | ||
| | .option("file_start_offset", "0") | Specifies the number of bytes to skip at the beginning of each file. | | ||
| | .option("file_end_offset", "0") | Specifies the number of bytes to skip at the end of each file. | | ||
| | .option("record_start_offset", "0") | Specifies the number of bytes to skip at the beginning of each record before applying copybook fields to data. | | ||
| | .option("record_end_offset", "0") | Specifies the number of bytes to skip at the end of each record after applying copybook fields to data. | | ||
| | .option("gpg_private_key", ascGpgKeyContent) | Specifies the ASCII-armored GPG private key for decrypting data files. | | ||
| | .option("gpg_private_key_passphrase", "passphrase") | Specifies the passphrase for the ASCII-armored GPG private key used for decrypting data files. If not specified, empty passphrase will be used. | |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use an h3 heading for this subsection.
Line [1697] jumps from ## Summary of all available options to ##### File reading options. Change it to ### File reading options so the document outline complies with markdownlint MD001.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 1699 - 1707, Change the “File reading options”
subsection heading in the README from level five to level three, using “### File
reading options” beneath “## Summary of all available options” to maintain the
required heading hierarchy.
Source: Linters/SAST tools
JaCoCo code coverage report - 'cobol-parser'
|
JaCoCo code coverage report - 'spark-cobol'
|
…raw stream if GPG decryption fails.
…e raw underlying stream is closed.
…s with indexes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/BufferedFSDataInputStream.scala`:
- Around line 143-155: Update the GPG initialization flow around openStream() so
rawStream remains stored on the instance after fileSystem.open(filePath),
including when post-open operations such as BaseStream.skip fail. Add a
dedicated constructor-initialization cleanup path that closes this stored
rawStream when any subsequent initialization throws, while preserving the
existing decryptStream failure cleanup.
- Around line 51-62: Update close() in BufferedFSDataInputStream so in.close()
is attempted before rawStream.close(), with each close wrapped in its own
independent Try to ensure both are attempted even if the first fails. Keep
clearing in and rawStream after the close attempts, including when the decrypted
stream close fails.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b422fece-4707-495f-a35a-7809d446c6e7
📒 Files selected for processing (3)
cobol-parser/src/main/scala/za/co/absa/cobrix/cobol/parser/Copybook.scalacobol-parser/src/test/scala/za/co/absa/cobrix/cobol/parser/extract/BinaryExtractorSpec.scalaspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/BufferedFSDataInputStream.scala
Comment on lines
+143
to
+155
| rawStream = fileSystem.open(filePath) | ||
| try { | ||
| GpgUtils.decryptStream(rawStream, keyring, gpgPassphrase.map(_.toCharArray).getOrElse(Array.emptyCharArray)) | ||
| } catch { | ||
| case ex: Throwable => | ||
| // Close rawStream only if decryptStream() fails to return a decrypted stream. Ignore errors that might happen on close. | ||
| Try { | ||
| if (rawStream != null) { | ||
| rawStream.close() | ||
| } | ||
| } | ||
| rawStream = null | ||
| throw ex |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the target file and related GpgUtils call sites.
target="spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/BufferedFSDataInputStream.scala"
if [ -f "$target" ]; then
echo "== target exists =="
wc -l "$target"
echo "== outline =="
ast-grep outline "$target" || true
echo "== relevant lines 1-230 =="
sed -n '1,230p' "$target" | nl -ba
else
echo "target not found: $target"
echo "candidate files:"
fd -i 'BufferedFSDataInputStream\.scala|GpgUtils' .
fi
echo "== DecryptStream references =="
rg -n "GpgUtils\.decryptStream|decryptStream\(|BufferedFSDataInputStream|class BufferedFSDataInputStream|def close\(|startOffset|skip|baseStream|rawStream" spark-cobol/src/main/scala -S || true
echo "== Other close patterns =="
rg -n "rawStream\s*=|baseStream\s*=|startOffset|GpgUtils\.decryptStream" spark-cobol/src/main/scala -S -C 3 || trueRepository: AbsaOSS/cobrix
Length of output: 484
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/BufferedFSDataInputStream.scala"
echo "== target lines =="
awk '{printf "%6d %s\n", NR, $0}' "$target"
echo "== decrypt/close/startSkip/openStream references =="
rg -n "GpgUtils\.decryptStream|decryptStream\(|BufferedFSDataInputStream|def close\(|startOffset|skip|baseStream|rawStream|openStream|new BufferedFSDataInputStream|case.*GpgUtils|GpgUtils" spark-cobol/src/main/scala -S -C 2
echo "== file list for GpgUtils =="
git ls-files | sed -n '/GpgUtils/p'Repository: AbsaOSS/cobrix
Length of output: 41274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== GpgUtils.scala relevant implementation =="
awk '{printf "%6d %s\n", NR, $0}' spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/utils/GpgUtils.scala | sed -n '1,120p'
echo "== FileReader / getStreams call site =="
awk '{printf "%6d %s\n", NR, $0}' spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/index/IndexBuilder.scala | sed -n '220,275p'Repository: AbsaOSS/cobrix
Length of output: 10120
Close the opened stream when constructor initialization throws.
rawStream := fileSystem.open(filePath) runs before BaseStream.skip for compressed GPG streams. If startOffset > 0 and skip throws, openStream() returns without assigning in, object construction exits, and close() is called on an object with rawStream == null. Keep rawStream on an instance field for the GPG branch and close it from a dedicated cleanup code path when any post-open initialization throws.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/BufferedFSDataInputStream.scala`
around lines 143 - 155, Update the GPG initialization flow around openStream()
so rawStream remains stored on the instance after fileSystem.open(filePath),
including when post-open operations such as BaseStream.skip fail. Add a
dedicated constructor-initialization cleanup path that closes this stored
rawStream when any subsequent initialization throws, while preserving the
existing decryptStream failure cleanup.
…DataInputStream.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test45GpgEncryptedFilesSpec.scala (1)
80-89: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnable the index path explicitly.
The scenario name says
"with indexes", but this block does not setenable_indexesto"true". It relies on the reader default. Add the explicit option, or verify thatinput_split_recordsalways enables indexing.Proposed option
.option("generate_record_id", "true") + .option("enable_indexes", "true") .option("input_split_records", 1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test45GpgEncryptedFilesSpec.scala` around lines 80 - 89, Update the test case “load normally a variable-record-length file with indexes” to explicitly set the reader’s enable_indexes option to "true" alongside the existing input_split_records option, ensuring the scenario directly exercises indexed reading rather than relying on defaults.data/test41_expected/test41c.txt (1)
1-10: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftMake the fixture exercise variable record lengths.
Every expected row has
Record_Byte_Lengthset to2202. The integration scenario is named"load normally a variable-record-length file with indexes". A fixed 2202-byte reader could therefore satisfy this expected output.Add records with different physical lengths and regenerate
data/test41_data/example.bin.gpganddata/test41_expected/test41c.txt. IfRecord_Byte_Lengthis not the physical input length for this format, add a direct assertion that the encrypted fixture has varying record boundaries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data/test41_expected/test41c.txt` around lines 1 - 10, Update the test41 fixture scenario so the encrypted input contains records with genuinely different physical lengths, then regenerate example.bin.gpg and test41c.txt so Record_Byte_Length reflects those lengths instead of always being 2202. If Record_Byte_Length does not represent physical input size, add a direct assertion in the variable-record-length integration test that the encrypted fixture has varying record boundaries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@data/test41_expected/test41c.txt`:
- Around line 1-10: Update the test41 fixture scenario so the encrypted input
contains records with genuinely different physical lengths, then regenerate
example.bin.gpg and test41c.txt so Record_Byte_Length reflects those lengths
instead of always being 2202. If Record_Byte_Length does not represent physical
input size, add a direct assertion in the variable-record-length integration
test that the encrypted fixture has varying record boundaries.
In
`@spark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test45GpgEncryptedFilesSpec.scala`:
- Around line 80-89: Update the test case “load normally a
variable-record-length file with indexes” to explicitly set the reader’s
enable_indexes option to "true" alongside the existing input_split_records
option, ensuring the scenario directly exercises indexed reading rather than
relying on defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1167e1f0-70e5-4abf-8af2-f0eb3f90ab3e
📒 Files selected for processing (3)
data/test41_expected/test41c.txtspark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/BufferedFSDataInputStream.scalaspark-cobol/src/test/scala/za/co/absa/cobrix/spark/cobol/source/integration/Test45GpgEncryptedFilesSpec.scala
🚧 Files skipped from review as they are similar to previous changes (1)
- spark-cobol/src/main/scala/za/co/absa/cobrix/spark/cobol/source/streaming/BufferedFSDataInputStream.scala
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #757
Summary by CodeRabbit
New Features
Bug Fixes
Tests