feat(config): add optional system assertions to TDFConfig #2316
Conversation
github-actions
bot
added
external-contributor
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @sujankota, 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, providing a summary of this pull request to help everyone quickly understand the changes.
This PR introduces a new configuration option, WithStandardAssertions, to the TDFConfig struct in the Go SDK. The primary goal is to provide a convenient way for users to include a predefined set of common and useful assertions in their TDF configurations, specifically adding a timestamp and metadata about the SDK version and the operating system/architecture where the TDF was created. This simplifies the process of adding these standard pieces of information to TDF files.
Highlights
- New Configuration Option: A new
TDFOptionfunction,WithStandardAssertions, has been added tosdk/tdf_config.go. This option allows users to easily apply a set of standard assertions when creating or configuring aTDFConfig. - Standard Assertions Defined: The
WithStandardAssertionsoption adds two specific assertions: one capturing the current timestamp (RFC3339 format) and another providing metadata about the SDK version and the Go runtime's OS and architecture. - Unit Testing: A new unit test,
TestWithStandardAssertions, has been added insdk/tdf_config_test.goto verify that the new option correctly adds the expected number and type of standard assertions with the appropriate properties.
Changelog
- sdk/tdf_config.go
- Added imports for
uuid,runtime, andtime. - Implemented the
WithStandardAssertionsfunction to add predefined timestamp and SDK/OS metadata assertions to the TDFConfig.
- Added imports for
- sdk/tdf_config_test.go
- Added
TestWithStandardAssertionsto validate the functionality of the newWithStandardAssertionsoption.
- 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.
Config options flow,
Standard facts now added in,
Code review begins.
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.
Code Review
This pull request introduces a WithStandardAssertions option for TDFConfig, which adds a predefined set of assertions, including a timestamp and SDK/OS information. The implementation is clear, and a new unit test, TestWithStandardAssertions, has been added to verify the basic functionality.
The changes are a good step towards providing useful default metadata. I have a few suggestions regarding constant usage and test completeness that could further enhance the code quality.
Summary of Findings
- Potential Typo in Constant Name: The constant
Paylaodused for assertion scope appears to be a typo forPayload. Correcting this at its definition would improve code clarity. - Usage of Magic Strings: String literals are used for assertion
FormatandSchemavalues (e.g., "string", "metadata", ""). Defining these as constants would enhance maintainability and readability. - Test Coverage for Option Interaction: The new unit test covers
WithStandardAssertionsin isolation. Expanding tests to include interactions withWithAssertionswould ensure options compose correctly. - Test Assertion Specificity (Low Severity): In
TestWithStandardAssertions, the check for the timestamp value (cfg.assertions[0].Statement.Value) usesassert.NotEmpty. A more robust check could involve attempting to parse the value withtime.Parse(time.RFC3339, ...). This was not added as a direct comment due to review severity settings. - Test Assertion Specificity for SDK/OS Info (Low Severity): In
TestWithStandardAssertions, the check for the SDK/OS info string (cfg.assertions[1].Statement.Value) usesassert.Contains(t, ..., "SDK-Version/"). A more precise check could compare against the fully expected formatted string usingTDFSpecVersion,runtime.GOOS, andruntime.GOARCH. This was not added as a direct comment due to review severity settings.
Merge Readiness
The pull request is a valuable addition for providing standard TDF assertions. The core functionality is well-implemented. However, there are a few medium-severity suggestions regarding a potential constant name typo, the use of magic strings, and enhancing test coverage for option interactions. Addressing these points would improve the code's maintainability and robustness. Therefore, I recommend addressing these changes before merging. As an AI, I am not authorized to approve pull requests; please ensure further review and approval by authorized team members.
sdk/tdf_config.go
Outdated
| { | ||
| ID: uuid.NewString(), | ||
| Type: BaseAssertion, | ||
| Scope: Paylaod, |
There was a problem hiding this comment.
The Scope for this assertion is set using the constant Paylaod. The name of this constant (which appears to be defined in sdk/assertion.go as Paylaod Scope = "payload") seems to contain a typo and might be intended as Payload.
If this is indeed a misspelling, could the constant's name be corrected at its definition (e.g., to AssertionScopePayload or simply Payload)? Subsequently, this line and line 242 would need to be updated to use the corrected constant name. This would improve overall code clarity and maintainability.
sdk/tdf_config.go
Outdated
| standardAssertions := []AssertionConfig{ | ||
| { | ||
| ID: uuid.NewString(), | ||
| Type: BaseAssertion, | ||
| Scope: Paylaod, | ||
| AppliesToState: Unencrypted, | ||
| Statement: Statement{ | ||
| Format: "string", | ||
| Schema: "", | ||
| Value: time.Now().Format(time.RFC3339), | ||
| }, | ||
| }, | ||
| { | ||
| ID: uuid.NewString(), | ||
| Type: BaseAssertion, | ||
| Scope: Paylaod, | ||
| AppliesToState: Unencrypted, | ||
| Statement: Statement{ | ||
| Format: "string", | ||
| Schema: "metadata", | ||
| Value: fmt.Sprintf("SDK-Version/%s OS/%s-%s", TDFSpecVersion, runtime.GOOS, runtime.GOARCH), | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
In the definition of standardAssertions, string literals like "string" (for Format), "" (for an empty Schema), and "metadata" (for Schema) are used directly.
To improve maintainability, reduce the risk of typos, and make the intent clearer, especially if these values are standard or used elsewhere, would it be beneficial to define them as named constants?
For example:
const (
AssertionFormatString = "string"
AssertionSchemaNone = "" // Or a more descriptive name if "" has specific meaning
AssertionSchemaMetadata = "metadata"
)These constants could then be used within the Statement structs, like Format: AssertionFormatString.
sdk/tdf_config_test.go
Outdated
| @@ -366,6 +366,30 @@ | |||
| } | |||
| } | |||
|
|
|||
| func TestWithStandardAssertions(t *testing.T) { | |||
There was a problem hiding this comment.
The new test TestWithStandardAssertions does a good job of verifying the WithStandardAssertions option in isolation.
To further ensure the robustness of how TDF options compose, have you considered adding scenarios to this test (or a new one) that check the interaction between WithStandardAssertions and the existing WithAssertions option? For instance:
- Applying
WithAssertionsfirst, thenWithStandardAssertions. - Applying
WithStandardAssertionsfirst, thenWithAssertions.
This would help confirm that assertions are always appended correctly and that one option doesn't inadvertently overwrite assertions from the other, regardless of application order.
Benchmark results, click to expandBenchmark 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 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to add a default assertion to TDFConfig. The changes include adding a new option WithDefaultAssertion, a function GetDefaultAssertionConfig to generate this default assertion, and integrating this into the TDF creation process. Unit tests have been added to verify the new functionality.
The implementation is straightforward, but there's one area for improvement regarding error handling, which I've detailed below.
Summary of Findings
- Error Handling: In
sdk/assertion.go, the error fromjson.MarshalinGetDefaultAssertionConfigis ignored. This could lead to an invalid JSON value in the assertion if marshalling fails. It's recommended to handle this error. - PR Metadata: The pull request description is minimal and the checklist is not filled out. Providing more details in the description and completing the checklist can help reviewers understand the changes and ensure all necessary steps have been taken. (Severity: low, not commented directly due to review settings)
- Test Clarity: In
Test_DefaultAssertions(sdk/tdf_test.go), line 499s.Require().NoError(err)appears to re-check an error already handled on line 495, or it might be using anerrvariable from a wider scope unintentionally. This could be confusing. Consider removing or clarifying this check. (Severity: low, not commented directly due to review settings)
Merge Readiness
The pull request introduces a useful feature for default assertions. The core logic is sound and well-tested. However, there is a medium-severity issue regarding error handling in sdk/assertion.go that should be addressed to improve robustness. Once this is resolved, the PR should be in good shape for merging. As an AI, I am not authorized to approve pull requests; please ensure other human reviewers approve these changes before merging.
| metadataJSON, _ := json.Marshal(metadata) | ||
|
|
||
| return AssertionConfig{ | ||
| ID: "default-assertion", | ||
| Type: BaseAssertion, | ||
| Scope: Paylaod, | ||
| AppliesToState: Unencrypted, | ||
| Statement: Statement{ | ||
| Format: "json", | ||
| Schema: "metadata", | ||
| Value: string(metadataJSON), | ||
| }, | ||
| } |
There was a problem hiding this comment.
The error returned by json.Marshal is currently being ignored. While marshalling the Metadata struct is unlikely to fail given its current simple structure (all string fields), it's a good practice to handle potential errors. If json.Marshal were to fail (e.g., due to future changes in the Metadata struct or unexpected system issues), metadataJSON would likely be nil. string(nil) results in an empty string "", which is not valid JSON. This could lead to an invalid assertion statement value being silently used.
Consider handling the error by, for example, logging it and defaulting to a valid empty JSON object string like "{}" for the Value field. This ensures the assertion remains structurally valid even if metadata generation fails.
metadataBytes, err := json.Marshal(metadata)
if err != nil {
// It's crucial to log this error for visibility, as it indicates an unexpected issue
// with marshalling a known-simple struct. This could point to deeper system problems
// or future incompatibilities if the Metadata struct evolves.
// e.g., log.Printf("Error marshalling default assertion metadata: %v", err) // Replace with actual logging mechanism
// Defaulting to an empty JSON object to maintain a valid assertion structure.
metadataBytes = []byte("{}")
}
return AssertionConfig{
ID: "default-assertion",
Type: BaseAssertion,
Scope: Paylaod, // This uses the existing constant `Paylaod`
AppliesToState: Unencrypted,
Statement: Statement{
Format: "json",
Schema: "metadata",
Value: string(metadataBytes),
},
}
There was a problem hiding this comment.
Missing constexpr.
Yeah, if this fails, you should panic IMO.
MustMarshall was declined as an extension: golang/go#38519
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 |
| metadataJSON, _ := json.Marshal(metadata) | ||
|
|
||
| return AssertionConfig{ | ||
| ID: "default-assertion", | ||
| Type: BaseAssertion, | ||
| Scope: Paylaod, | ||
| AppliesToState: Unencrypted, | ||
| Statement: Statement{ | ||
| Format: "json", | ||
| Schema: "metadata", | ||
| Value: string(metadataJSON), | ||
| }, | ||
| } |
There was a problem hiding this comment.
Missing constexpr.
Yeah, if this fails, you should panic IMO.
MustMarshall was declined as an extension: golang/go#38519
| // Populate the metadata | ||
| metadata := Metadata{ | ||
| TDFSpecVersion: TDFSpecVersion, | ||
| CreationDate: time.Now().Format(time.RFC3339), |
There was a problem hiding this comment.
Wait, do we not have a GetMetadata function? Which maybe also could error out...
| @@ -217,6 +219,14 @@ func WithSegmentSize(size int64) TDFOption { | |||
| } | |||
| } | |||
|
|
|||
| // WithDefaultAssertion returns an Option that adds a default assertion to the TDF. | |||
| func WithDefaultAssertion() TDFOption { | |||
There was a problem hiding this comment.
From the ticket,
- This option must allow developers to provide a predefined set of assertions (key-value pairs or similar structure).
- The mechanism should support adding common metadata types, such as:
- SDK Version used for creation
- Policy Enforcement Point (PEP) Name and Version (if applicable in the context)
- TDF Creation Timestamp
- Other relevant contextual information specific to the generating application (like DSP).
So we probably need to allow adding one or more, and also it should be a list or set
strantalis
changed the title
|
reopened under #2446 |
Proposed Changes
WithDefaultAssertion()Option to add default assertion.Checklist
Testing Instructions