feat(server)!: update server logic to handle empty topics arrays explicitly #1887
Conversation
Refactors metadata update tests to include subtests for various scenarios, such as empty, nil, and multiple topics. Improves test clarity and robustness by isolating cases. Updates server logic to handle empty topics arrays explicitly, ensuring consistent behavior when topics are empty or nil.
There was a problem hiding this comment.
Pull Request Overview
This PR implements explicit handling for empty topics arrays in project metadata updates, addressing a proto3 limitation where optional repeated fields cannot be distinguished from intentionally empty arrays. The solution uses a sentinel value (single empty string) to signal topic deletion while preserving backward compatibility.
Key Changes:
- Added server-side logic to detect and handle the empty topics sentinel value (array with single empty string)
- Refactored metadata update tests into isolated subtests covering nil, empty, normal, and multiple topics scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/internal/adapter/internalapi/server.go | Adds logic to convert sentinel value [""] to empty array for topic deletion |
| server/e2e/proto_project_metadata_test.go | Restructures tests into subtests for different topics scenarios (normal, empty, nil, multiple) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| // A single empty string is used as a sentinel value to signal topic deletion. | ||
| // This distinguishes it from proto3's default empty array behavior. | ||
| if len(req.Topics) == 1 && req.Topics[0] == "" { |
There was a problem hiding this comment.
@wilfredmulenga
I’d suggest wrapping this field in an optional message (like optional Topics topics) so we can clearly distinguish between “not provided” and “explicitly empty”. It’s easier to handle cases where we want to clear the list.
This might be a breaking change, so I’ll leave the final decision up to you.
now
message UpdateRequest {
repeated string topics = 1;
}can’t tell the difference between “not set” and “empty array,”
so we end up relying on sentinel values like [""] to represent clearing.
suggestion
message UpdateRequest {
optional Topics topics = 1;
}
message Topics {
repeated string values = 1;
}With this structure, we can distinguish:
• topics not provided → no update
• topics.values empty → explicitly clear the list
• topics.values non-empty → replace with new values
There was a problem hiding this comment.
Thanks. Thats a great approach. I have implemented it
wilfredmulenga
changed the title
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds logic to filter out duplicate and empty topic strings in project metadata updates, ensuring a clean and unique list of topics. Updates unit tests to validate this behavior.
Topicsto UpdateProjectMedata gRPC method call.valuesas below:Whilst current visualizer expects
topicsas an array as below:This will result in an error
onlywhentopicsfield is passed in the method and no error is it isn't. So the breaking change is minimal.