feat(rust): support pagination overrides with modules config#4077
feat(rust): support pagination overrides with modules config#4077suzmue wants to merge 1 commit intogoogleapis:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4077 +/- ##
=======================================
Coverage 81.93% 81.93%
=======================================
Files 78 78
Lines 6516 6516
=======================================
Hits 5339 5339
Misses 832 832
Partials 345 345 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 adds support for propagating pagination overrides for Rust modules. A logical issue was found where library-level pagination overrides are duplicated for every module within a library, leading to incorrect configuration. The current test case also needs improvement as it doesn't cover this scenario. Please address the duplication bug in moduleToModelConfig and enhance the test case to ensure correct functionality for libraries with multiple modules.
| if len(library.Rust.PaginationOverrides) > 0 { | ||
| modelCfg.PaginationOverrides = make([]sidekickconfig.PaginationOverride, len(library.Rust.PaginationOverrides)) | ||
| for i, override := range library.Rust.PaginationOverrides { | ||
| modelCfg.PaginationOverrides[i] = sidekickconfig.PaginationOverride{ | ||
| ID: override.ID, | ||
| ItemField: override.ItemField, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic adds the library-level PaginationOverrides to the model config for each module. Since moduleToModelConfig is called from a loop that iterates over all modules in a library, this will cause the same set of overrides to be duplicated for each module.
These overrides are defined at the library level and should be applied only once. Applying them to every module's config will result in incorrect, duplicated configuration downstream.
Please adjust the logic to ensure these overrides are not duplicated. The overrides should be handled once at the library level, rather than inside this per-module function.
| { | ||
| name: "with pagination overrides in rust module", | ||
| library: &config.Library{ | ||
| Name: "google-cloud-example", | ||
| Rust: &config.RustCrate{ | ||
| PaginationOverrides: []config.RustPaginationOverride{ | ||
| { | ||
| ID: ".google.cloud.example.v1.Example.ListExamples", | ||
| ItemField: "examples", | ||
| }, | ||
| }, | ||
| Modules: []*config.RustModule{ | ||
| { | ||
| Template: "prost", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| want: &parser.ModelConfig{ | ||
| Language: "rust", | ||
| Source: map[string]string{ | ||
| "googleapis-root": absPath(t, googleapisRoot), | ||
| "roots": "googleapis", | ||
| }, | ||
| PaginationOverrides: []sidekickconfig.PaginationOverride{ | ||
| { | ||
| ID: ".google.cloud.example.v1.Example.ListExamples", | ||
| ItemField: "examples", | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This new test case only covers the scenario with a single module, which masks a duplication bug in moduleToModelConfig. The library-level PaginationOverrides are incorrectly applied to every module, leading to duplicates.
The test runner logic (lines 847 and 852-854) will collect these duplicates. Please strengthen this test case by adding a second module, which will cause the test to fail and properly validate the fix.
Example update:
Modules: []*config.RustModule{
{
Template: "prost",
},
{
Template: "prost", // Add a second module
},
},1 participant
Pagination overrides need to be propagated when converting from a modules configuration for rust. This feature was already available for crates defined using library configuration, but is missing for modules.
Fixes #4076