-
Notifications
You must be signed in to change notification settings - Fork 24
feat(sdk): adds seeker interface to TDF Reader #2385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @imdominicreed, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team, Gemini here with a summary of this pull request. This PR introduces the io.Seeker
interface to the TDF Reader
struct. The primary goal is to allow users to reposition the read cursor within a TDF file, enabling operations like seeking to a specific offset before reading. This involves adding the Seek
method and modifying the existing WriteTo
method to respect the cursor position. Additionally, some minor adjustments were made to the ReadAt
method's segment calculation and loop logic, and corresponding tests were updated/added.
Highlights
- Update WriteTo to support seeking: The
WriteTo
method has been modified to start reading and decrypting from the segment corresponding to the current cursor position. It also handles starting mid-segment if the cursor is not at the beginning of a segment. - Refactor ReadAt segment calculation: The calculation for the start and end segment indices in the
ReadAt
method was refactored to use integer division instead of floating-point arithmetic withmath.Floor
andmath.Ceil
. - Updated tests: The
Test_TDFReader
test suite was updated and replaced to include test cases specifically for the newSeek
andWriteTo
functionality, in addition to the existingReadAt
tests.
Changelog
Click here to see the changelog
- sdk/tdf.go
- Removed unused
math
import (lines 10-16). - Added
Seek
method toReader
implementingio.Seeker
(lines 772-788). - Modified
WriteTo
method to track data index (dataIdx
) and skip segments before the cursor position (lines 804-808). - Modified
WriteTo
method to handle starting mid-segment by slicing the decrypted buffer (lines 840-844). - Updated cursor (
r.cursor
) and data index (dataIdx
) within theWriteTo
loop (lines 856-857). - Refactored segment index calculation in
ReadAt
to use integer division (lines 882-883). - Modified the loop termination condition in
ReadAt
, which now breaks before processing the segment atlastSegment
(lines 899-901). This differs from the previous logic (lines 913-916) which broke after processing the last segment, and might cause the last required segment to be skipped.
- Removed unused
- sdk/tdf_test.go
- Added
whence
andwriterOffset
fields to theTestReadAt
struct forSeek
/WriteTo
testing (lines 257-258). - Replaced the old
Test_TDFReader
function with a new one that includes test cases forSeek
followed byWriteTo
, alongsideReadAt
tests (lines 486-599). - Removed the old
Test_TDFReader
function (lines 1207-1308).
- Added
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A reader seeks its place,
Through encrypted data's space.
With cursor set right,
It reads with new might,
A byte stream finds its grace.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces the Seek
method to the TDF Reader
, enabling seeking within the decrypted content. This is a valuable addition for handling larger TDF files more efficiently. The implementation includes updates to WriteTo
and ReadAt
to respect the cursor position set by Seek
. The changes are generally well-structured, but there are a couple of areas related to correctness and efficiency that could be improved.
Summary of Findings
- Seek method correctness: The
Seek
method does not handle negative offsets or seeking beyond the end of the file according to theio.Seeker
interface contract. This is a high-severity correctness issue. - Seek method efficiency: The implementation of
Seek(..., io.SeekEnd)
calculates the total file size by iterating through all segments, which can be inefficient for large files with many segments. This is a medium-severity efficiency issue. - WriteTo logic clarity: The logic for handling the starting offset in
WriteTo
is correct but could be slightly clearer with more descriptive variable names. This is a medium-severity maintainability issue. - ReadAt segment calculation: The change to use integer arithmetic for calculating start and end segments in
ReadAt
is a good improvement for efficiency and correctness. This was a positive change. - Test coverage for Seek: While new tests were added for
Seek
andWriteTo
after seeking, they do not cover edge cases like negative offsets or seeking far beyond the end of the file. (Note: Per review settings, this is a low-severity issue and was not commented on directly in the code).
Merge Readiness
The pull request introduces a necessary Seek
interface and updates related read methods. However, the identified high-severity correctness issues in the Seek
implementation (handling invalid offsets and seeking beyond the end) should be addressed before merging. The medium-severity issues related to SeekEnd
efficiency and WriteTo
clarity are also recommended for improvement. Please address the high-severity issues at a minimum. I am unable to approve the pull request; please have others review and approve this code before merging.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
🤖 I have created a release *beep* *boop* --- ## [0.5.0](sdk/v0.4.7...sdk/v0.5.0) (2025-06-23) ### Features * add system metadata assertions to TDFConfig ([#2446](#2446)) ([4eb9fff](4eb9fff)) * **core:** DSPX-608 - Deprecate public_client_id ([#2185](#2185)) ([0f58efa](0f58efa)) * **sdk:** adds seeker interface to TDF Reader ([#2385](#2385)) ([63ccd9a](63ccd9a)) * **sdk:** Allow key splits with same algo ([#2454](#2454)) ([7422b15](7422b15)) * **sdk:** autoconfig kaos with kids ([#2438](#2438)) ([c272016](c272016)) * **sdk:** Enable base key support. ([#2425](#2425)) ([9ff3806](9ff3806)) ### Bug Fixes * **ci:** Fix intermittent failures from auth tests ([#2345](#2345)) ([395988a](395988a)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.3.6 to 0.4.0 in /sdk ([#2397](#2397)) ([99e3aa4](99e3aa4)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.4.0 to 0.5.0 in /sdk ([#2471](#2471)) ([e8f97e0](e8f97e0)) * **deps:** bump the external group across 1 directory with 5 updates ([#2400](#2400)) ([0b7ea79](0b7ea79)) * set consistent system metadata id and schema ([#2451](#2451)) ([5db3cf2](5db3cf2)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [0.7.0](sdk/v0.6.1...sdk/v0.7.0) (2025-08-25) ### ⚠ BREAKING CHANGES * **core:** Require go 1.23+ ([#1979](#1979)) ### Features * add system metadata assertions to TDFConfig ([#2446](#2446)) ([4eb9fff](4eb9fff)) * **authz:** authz v2 versioning implementation ([#2173](#2173)) ([557fc21](557fc21)) * **core:** adds bulk rewrap to sdk and service ([#1835](#1835)) ([11698ae](11698ae)) * **core:** Adds EC withSalt options ([#2126](#2126)) ([67b6fb8](67b6fb8)) * **core:** Adds ErrInvalidPerSchema ([#1860](#1860)) ([456639e](456639e)) * **core:** DSPX-608 - Deprecate public_client_id ([#2185](#2185)) ([0f58efa](0f58efa)) * **core:** EXPERIMENTAL: EC-wrapped key support ([#1902](#1902)) ([652266f](652266f)) * **core:** Expose version info ([#1841](#1841)) ([92a9f5e](92a9f5e)) * **core:** Require go 1.23+ ([#1979](#1979)) ([164c922](164c922)) * **core:** v2 ERS with proto updates ([#2210](#2210)) ([a161ef8](a161ef8)) * **policy:** actions service RPCs should actually hit storage layer CRUD ([#2063](#2063)) ([da4faf5](da4faf5)) * **policy:** Add list key mappings rpc. ([#2533](#2533)) ([fbc2724](fbc2724)) * **policy:** adds new public keys table ([#1836](#1836)) ([cad5048](cad5048)) * **policy:** Allow the deletion of a key. ([#2575](#2575)) ([82b96f0](82b96f0)) * **policy:** Default Platform Keys ([#2254](#2254)) ([d7447fe](d7447fe)) * **policy:** DSPX-902 NDR service crud implementation (2/2) ([#2066](#2066)) ([030ad33](030ad33)) * **policy:** key management crud ([#2110](#2110)) ([4c3d53d](4c3d53d)) * **sdk:** Add a KAS allowlist ([#2085](#2085)) ([d7cfdf3](d7cfdf3)) * **sdk:** add nanotdf plaintext policy ([#2182](#2182)) ([e5c56db](e5c56db)) * **sdk:** adds seeker interface to TDF Reader ([#2385](#2385)) ([63ccd9a](63ccd9a)) * **sdk:** Allow key splits with same algo ([#2454](#2454)) ([7422b15](7422b15)) * **sdk:** Allow schema validation during TDF decrypt ([#1870](#1870)) ([b7e6fb2](b7e6fb2)) * **sdk:** autoconfig kaos with kids ([#2438](#2438)) ([c272016](c272016)) * **sdk:** bump protocol/go v0.6.0 ([#2536](#2536)) ([23e4c2b](23e4c2b)) * **sdk:** CreateTDF option to run with specific target schema version ([#2045](#2045)) ([0976b15](0976b15)) * **sdk:** Enable base key support. ([#2425](#2425)) ([9ff3806](9ff3806)) * **sdk:** Expose connectrpc wrapper codegen for re-use ([#2322](#2322)) ([8b29392](8b29392)) * **sdk:** MIC-1436: User can decrypt TDF files created with FileWatcher2.0.8 and older. ([#1833](#1833)) ([f77d110](f77d110)) * **sdk:** remove hex encoding for segment hash ([#1805](#1805)) ([d7179c2](d7179c2)) * **sdk:** sdk.New should validate platform connectivity and provide precise error ([#1937](#1937)) ([aa3696d](aa3696d)) * **sdk:** Use ConnectRPC in the go client ([#2200](#2200)) ([fc34ee6](fc34ee6)) ### Bug Fixes * Allow parsing IPs as hostnames ([#1999](#1999)) ([d54b550](d54b550)) * **ci:** Fix intermittent failures from auth tests ([#2345](#2345)) ([395988a](395988a)) * **ci:** Update expired ca and certs in oauth unit tests ([#2113](#2113)) ([5440fcc](5440fcc)) * **core:** Autobump sdk ([#1863](#1863)) ([855cb2b](855cb2b)) * **core:** Autobump sdk ([#1873](#1873)) ([085ac7a](085ac7a)) * **core:** Autobump sdk ([#1894](#1894)) ([201244e](201244e)) * **core:** Autobump sdk ([#1917](#1917)) ([edeeb74](edeeb74)) * **core:** Autobump sdk ([#1941](#1941)) ([0a5a948](0a5a948)) * **core:** Autobump sdk ([#1948](#1948)) ([4dfb457](4dfb457)) * **core:** Autobump sdk ([#1968](#1968)) ([7084061](7084061)) * **core:** Autobump sdk ([#1972](#1972)) ([7258f5d](7258f5d)) * **core:** Autobump sdk ([#2102](#2102)) ([0315635](0315635)) * **core:** Fixes protoJSON parse bug on ec rewrap ([#1943](#1943)) ([9bebfd0](9bebfd0)) * **core:** Improves errors when under heavy load ([#2132](#2132)) ([4490a14](4490a14)) * **core:** Update fixtures and flattening in sdk and service ([#1827](#1827)) ([d6d6a7a](d6d6a7a)) * **core:** Updates ec-wrapped to newer salt ([#1961](#1961)) ([0e17968](0e17968)) * **deps:** bump github.com/docker/docker from 28.2.2+incompatible to 28.3.3+incompatible in /sdk ([#2597](#2597)) ([a68d00d](a68d00d)) * **deps:** bump github.com/opentdf/platform/lib/ocrypto from 0.2.0 to 0.3.0 in /sdk ([#2502](#2502)) ([3ec8b35](3ec8b35)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.3.6 to 0.4.0 in /sdk ([#2397](#2397)) ([99e3aa4](99e3aa4)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.4.0 to 0.5.0 in /sdk ([#2471](#2471)) ([e8f97e0](e8f97e0)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.5.0 to 0.5.1 in /sdk ([#2505](#2505)) ([4edab72](4edab72)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.6.0 to 0.6.2 in /sdk ([#2586](#2586)) ([4ed9856](4ed9856)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.6.2 to 0.7.0 in /sdk ([#2627](#2627)) ([e775e14](e775e14)) * **deps:** bump golang.org/x/oauth2 from 0.26.0 to 0.30.0 in /sdk ([#2252](#2252)) ([9b775a2](9b775a2)) * **deps:** bump google.golang.org/grpc from 1.71.0 to 1.72.1 in /sdk ([#2244](#2244)) ([49484e0](49484e0)) * **deps:** bump the external group across 1 directory with 5 updates ([#2400](#2400)) ([0b7ea79](0b7ea79)) * **deps:** bump toolchain in /lib/fixtures and /examples to resolve CVE GO-2025-3563 ([#2061](#2061)) ([9c16843](9c16843)) * Improve http.Client usage for security and performance ([#1910](#1910)) ([e6a53a3](e6a53a3)) * **sdk:** adds connection options to getPlatformConfiguration ([#2286](#2286)) ([a3af31e](a3af31e)) * **sdk:** Allow reuse of session key ([#2016](#2016)) ([d48c11e](d48c11e)) * **sdk:** bump lib/ocrypto to 0.1.8 ([#1938](#1938)) ([53fa8ab](53fa8ab)) * **sdk:** bump protocol/go module dependencies ([#2078](#2078)) ([e027f43](e027f43)) * **sdk:** Display proper error on kas rewrap failure ([#2081](#2081)) ([508cbcd](508cbcd)) * **sdk:** everything is `mixedSplits` now ([#1861](#1861)) ([ba78f14](ba78f14)) * **sdk:** Fix compatibility between bulk and non-bulk rewrap ([#1914](#1914)) ([74abbb6](74abbb6)) * **sdk:** Fixed token expiration time ([#1854](#1854)) ([c3cda1b](c3cda1b)) * **sdk:** perfsprint lint issues ([#2208](#2208)) ([d36a078](d36a078)) * **sdk:** Prefer KID and Algorithm selection from key maps ([#2475](#2475)) ([98fd392](98fd392)) * **sdk:** Removes unnecessary down-cast of `int` ([#1869](#1869)) ([66f0c14](66f0c14)) * **sdk:** Version config fix ([#1847](#1847)) ([be5d817](be5d817)) * Service utilize `httputil.SafeHttpClient` ([#1926](#1926)) ([af32700](af32700)) * set consistent system metadata id and schema ([#2451](#2451)) ([5db3cf2](5db3cf2)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Proposed Changes
WriteTo
. This is useful when the TDF is large but only want a specific segment from the data.ReadAt
had an off by one error decrypting one extra segment when it was not required to do so.Checklist
Testing Instructions