Add support for multiple downloader configs #27877
Conversation
UrlRewriter does not used the passed in log arguement in its contructor. This should have been cleaned up as part of bazelbuild@d21345d This also cleans up some other routing to provide that logger.
You can now set the `--downloader_config` flag multiple times to specify several downloader config files at once. Fixes bazelbuild#13131
github-actions
bot
added
team-ExternalDeps
Summary of ChangesHello @willstranton, 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 significantly enhances the flexibility of the downloader configuration by enabling the use of multiple configuration files. This allows for more modular and layered URL rewriting and blocking rules, addressing a long-standing issue and streamlining the setup for complex download scenarios. The underlying code has been updated to gracefully handle these multiple inputs. Highlights
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 adds support for specifying multiple downloader configuration files by allowing the --downloader_config flag to be used multiple times. The implementation correctly handles parsing multiple configuration files and aggregating their rules. The refactoring to remove unused arguments from UrlRewriter also improves code clarity.
My review includes two high-severity findings in UrlRewriter.java:
- A potential
NullPointerExceptionwhen resolving config paths if Bazel is run outside a workspace. - An incorrect error handling pattern when using
com.google.common.io.Closer, which could obscure the root cause of file I/O errors.
I've provided suggestions to fix both issues. Otherwise, the changes look good and well-tested.
| public static UrlRewriter getDownloaderUrlRewriter( | ||
| Path workspaceRoot, @Nullable PathFragment configPath, Reporter reporter) | ||
| Path workspaceRoot, @Nullable List<PathFragment> configPaths) | ||
| throws UrlRewriterParseException { |
There was a problem hiding this comment.
workspaceRoot can be null if Bazel is run outside a workspace. However, on line 106, workspaceRoot.getRelative(...) is called without a null check, which will cause a NullPointerException if configPaths is not empty.
You should add a check to handle this case. For example, you could throw an exception if workspaceRoot is null and configPaths is not empty, as relative paths for downloader configs cannot be resolved without a workspace.
There was a problem hiding this comment.
This was the existing code. I'll leave this as is.
| } catch (Throwable e) { | ||
| throw closer.rethrow(new UrlRewriterParseException(e.getMessage())); | ||
| } finally { |
There was a problem hiding this comment.
The use of closer.rethrow here is incorrect. It wraps the new UrlRewriterParseException in a RuntimeException, which is not what the method signature indicates. It also loses the original exception's stack trace, making debugging harder. The original exception should be rethrown so it can be caught by the outer catch block.
This will rethrow IOExceptions from getInputStream, which will then be caught by the outer catch block on line 138 and wrapped in a UrlRewriterParseException, preserving the cause. This is the standard pattern for using Closer.
| } catch (Throwable e) { | |
| throw closer.rethrow(new UrlRewriterParseException(e.getMessage())); | |
| } finally { | |
| } catch (Throwable e) { | |
| throw closer.rethrow(e); | |
There was a problem hiding this comment.
Thanks - you're right. This should just rethrow. Updated in the latest commit.
|
|
||
| @VisibleForTesting | ||
| UrlRewriter(Consumer<String> log, String filePathForErrorReporting, Reader reader) | ||
| UrlRewriter(String filePathForErrorReporting, Reader reader) |
There was a problem hiding this comment.
Why do we still need this one?
There was a problem hiding this comment.
It isn't needed. I didn't want to update all the tests and left this convenience constructor.
It is now removed in the latest commit pushed. I also removed an unneeded instance variable as well. Please take a look.
The single file and reader constructor of UrlRewriter is removed in favor of the more general multiple file/reader constructor. The `rewriter` instance variable is also removed since it is always assigned to the same value.
2 participants
You can now set the
--downloader_configflag multiple times to specify several downloader config files at once.Fixes #13131
An initial refactoring was done to clean up unused arguments in
UrlRewriter.