feat(internal/librarian): populate Java Maven coordinates from defaults#6554
feat(internal/librarian): populate Java Maven coordinates from defaults#6554JoeWang1127 wants to merge 29 commits into
Conversation
![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 introduces support for mapping API paths to Java group IDs by adding the api_path_to_group_id configuration field and implementing the fillJava function to populate Java-specific defaults (ArtifactID and GroupID) based on matching API path prefixes. The review feedback highlights several critical issues in the fillJava implementation, including non-deterministic prefix matching due to map iteration, potential false matches from path boundary issues, overwriting of existing ArtifactIDs, and the omission of defaulting GenerateGRPC to true. A refactored version of fillJava was suggested to address these concerns.
| if d.Go != nil { | ||
| return fillGo(lib, d) | ||
| } | ||
| if d.Java != nil { |
There was a problem hiding this comment.
While you are following the pattern of the surrounding code, the surrounding code is wrong.
At the lowest level, a sequence of mutually exclusive if statements like this should be a switch instead.
At a higher level, why do we have a bunch of fields that seem to be mutually exclusive, do we actually verify that they are somewhere, if not does the order in which they are checked matter, why does Java get higher priority than rust... the fact that all these questions exist implies a fundamental design fault with another API somewhere (probably whatever config.Default is or whatever builds it).
At first glance it looks like there should be a single field in that thing, of an interface that supports a Fill method, rather than what seems like weird type switch and method dispatch block in this code.
There was a problem hiding this comment.
As a first step, I've refactor fillDefaults to use a switch.
I'll create follow up PRs to refactor config.Default struct.
| lib.Java = &config.JavaModule{} | ||
| } | ||
| if lib.Java.GroupID != "" || d.Java.CustomGroupIDs == nil { | ||
| return lib |
There was a problem hiding this comment.
This seems like a footgun
This method reads like one that is allowed to set multiple fields to their default values, but this return makes the assumption that GroupID is the only field that this will ever be true for.
If I was adding a new default later one I would add it at the bottom of the function and be very confused why it was not always being set...
There was a problem hiding this comment.
Created a helper function to fill group id.
This can make the function easily extend to fill other fields for java library.
| } | ||
| } | ||
| if lib.Java.GroupID != "" { | ||
| break |
There was a problem hiding this comment.
Adding an outer break that depends on an inner break triggering is a code smell, it almost always implies some code that should be extracted to a function and the breaks should be returns instead.
There was a problem hiding this comment.
Done, replaced the inner break to return.
ianthehat
left a comment
ianthehat
left a comment
There was a problem hiding this comment.
One very minor comment, cl is good apart from that.
| // fillGroupIDIfEmpty sets the Java group ID on lib if one is not already configured. | ||
| // It matches the library's API paths against the custom group ID prefixes in default | ||
| // and assigns the first matching group ID. | ||
| func fillGroupIDIfEmpty(lib *config.Library, d *config.Default) *config.Library { |
There was a problem hiding this comment.
No point returning the lib, it is always the same as the one passed in, don't make the reader guess wether there is a good reason for that.
3 participants
The library configuration loader is updated to automatically populate Java Maven coordinates (GroupID and ArtifactID) based on API path prefix mappings defined in default configuration.
When a library's Java module configuration is missing or its GroupID is empty, the loader scans the library's API paths against the default api_path_to_group_id map to resolve the appropriate GroupID and sets the ArtifactID using the standard "google-" naming convention.
Add default mappings in follow up PRs.
For #6513