Skip to content

Conversation

eugenioenko
Copy link
Contributor

@eugenioenko eugenioenko commented Aug 14, 2025

Motivation
When creating a collection a source is required but for collection that does not make sense. In the collection the source is passed on the .encrypt() method

const collection = await opentdf.createNanoTDFCollection({
      platformUrl,
      // TODO: source is marked as required, but we don't need it here
      source: {} as never,
    });

    const stream = await collection.encrypt({
      type: "buffer",
      location: new TextEncoder().encode(inputText),
    });

    const encrypted = await new Response(stream).bytes();

PR Changes

  • make source optional
  • add TODO to verify if it can be removed directly instead of just making it optional

Note
The overide won't work, we need to modify the parent type (which we might not want to do)

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @eugenioenko, 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!

This pull request refines the CreateNanoTDFCollectionOptions type by making the source property optional. This change addresses an inconsistency where source was a required field during collection creation but is only truly utilized later during the encryption process. The update improves the API's clarity and reduces the need for placeholder values, while also adding a note for potential future removal of the property from the creation options.

Highlights

  • Type Definition Update: The source property in the CreateNanoTDFCollectionOptions type has been changed from a required property to an optional one. This aligns the type definition with the actual usage pattern where source is passed during the .encrypt() method call, not necessarily during collection creation.
  • Future Refactoring Note: A TODO comment has been added to the source property's JSDoc, prompting a future review to determine if the source property can be completely removed from the CreateNanoTDFCollectionOptions if it's truly not needed at that stage.
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

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.

Footnotes

  1. 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.

Copy link

@gemini-code-assist gemini-code-assist bot left a 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 aims to make the source property optional when creating a NanoTDF collection, which is a good improvement as the source is only needed for the encrypt method. However, the current implementation doesn't achieve this due to how TypeScript intersection types work. I've left a comment explaining the issue and suggesting a fix that involves refactoring the type definition for CreateNanoTDFCollectionOptions.

Comment on lines +103 to +104
/** Optional source. TODO: check if it can be removed from create options */
source?: Source;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This change is intended to make the source property optional, but it won't have the desired effect. CreateNanoTDFCollectionOptions is defined as an intersection with CreateNanoTDFOptions, which inherits a required source property from CreateOptions. In a TypeScript intersection, a required property remains required even if intersected with a type where it's optional.

The correct way to achieve this is to modify the definition of CreateNanoTDFCollectionOptions to use Omit<CreateNanoTDFOptions, 'source'>. This would properly remove the required source property. The source?: Source could then be added back to allow it to be passed through from createNanoTDF without being mandatory for direct createNanoTDFCollection calls.

Example of a corrected definition:

export type CreateNanoTDFCollectionOptions = Omit<CreateNanoTDFOptions, 'source'> & {
  platformUrl: string;
  maxKeyIterations?: number;
  source?: Source;
};

This change would need to be applied to the definition of CreateNanoTDFCollectionOptions on line 98, which is outside the current diff.

Copy link

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.

1 participant