fix!: Make tenant optional in MessageSendParams#694
fix!: Make tenant optional in MessageSendParams#694CokeLee777 wants to merge 1 commit intoa2aproject:mainfrom
Conversation
Summary of ChangesHello, 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 enhances the flexibility of the Highlights
Changelog
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes the tenant field in MessageSendParams optional. The changes correctly apply the @Nullable annotation, remove the null-check in the constructor, and update the builder method and Javadoc. My review includes a suggestion to ensure consistency in handling null tenants across the class, particularly in the builder and convenience constructor, to fully align with the goal of making the tenant optional.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request makes the tenant field in MessageSendParams optional. The changes correctly update the record definition, constructors, and builder to allow a null tenant. However, the compact constructor normalizes a null tenant to an empty string. The feedback suggests reconsidering this normalization to keep the behavior consistent with the @Nullable annotation, which implies that the field can hold a null value. This comment is retained as it does not conflict with any provided rules.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request makes the tenant field in MessageSendParams optional, which was previously a required field. The changes involve removing the null-check in the compact constructor, updating the builder to accept a nullable tenant, and normalizing null tenants to an empty string in the convenience constructor and builder. The Javadocs have also been updated to reflect these changes.
My review identifies one area for improvement to ensure the tenant is consistently non-null. I've suggested a change to the compact constructor to handle null normalization, which will make the class more robust by ensuring the tenant field is never null, regardless of how the MessageSendParams object is instantiated.
The `tenant` field in `MessageSendParams` was previously required and
validated as non-null in the compact constructor, which forced callers
to always provide a tenant even when it is not applicable.
This change makes `tenant` truly optional:
- Remove `Assert.checkNotNullParam("tenant", tenant)` from the compact
constructor so that null values are accepted at construction time
- Annotate the `tenant` record component with `@Nullable` to clearly
signal that null may be passed by callers
- Normalize null tenant to an empty string in the compact constructor,
which is the single point of normalization for all construction paths
- Update `Builder.tenant()` to accept a `@Nullable String` so that
callers can explicitly pass null without a type violation
- Simplify `Builder.build()` to pass `tenant` directly, delegating
normalization to the compact constructor
Javadoc is updated across the class to accurately reflect the optional
nature, default value, and normalization behavior of `tenant`.
Fixes: a2aproject#693
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to make the tenant field in MessageSendParams optional, which is a good improvement. However, the current implementation of normalizing the tenant in a compact constructor will cause a compilation error because compact constructor parameters are final and cannot be reassigned. I've provided a critical comment with a suggested fix that uses a canonical constructor to perform the normalization, which is the correct approach for this scenario. This ensures the code is correct and maintains the desired invariant.
|
I think this is wrong: the tenant is a path parameter where "" does make sense. Having something that can be null requires testing at each usage. Also the same pattern is repeated across all the source code. So I disagree with this PR |
2 participants
Description
The
tenantfield inMessageSendParamswas previously required and validated as non-null in the compact constructor, which forced callers to always provide a tenant even when it is not applicable.This change makes
tenanttruly optional:Assert.checkNotNullParam("tenant", tenant)from the compactconstructor so that null values are accepted at construction time
tenantrecord component with@Nullableto clearlysignal that null may be passed by callers
which is the single point of normalization for all construction paths
Builder.tenant()to accept a@Nullable Stringso thatcallers can explicitly pass null without a type violation
Builder.build()to passtenantdirectly, delegatingnormalization to the compact constructor
Javadoc is updated across the class to accurately reflect the optional nature, default value, and normalization behavior of
tenant.Fixes: #693