[LCHIB-562] Max days override #991
Conversation
This makes it easier for us to retroactively collect and repopulate the commits data from a prospect after they incorrectly configure shallow-clone.
WalkthroughThe changes modify the Changes
Sequence Diagram(s)sequenceDiagram
participant CGC as CommitGraphCollector
participant S as Server
CGC->>S: Send HTTP GET request
S-->>CGC: Return HTTP response (latestResponse)
CGC->>CGC: Call honorMaxDaysHeader(latestResponse)
Note right of CGC: Check for "X-Max-Days" header and update maxDays
CGC->>CGC: Call getAdvertisedRefs(latestResponse)
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/launchableinc/ingest/commits/CommitGraphCollector.java (2)
179-179: Fix code style: add space after if.The code style is inconsistent with the rest of the file: there should be a space after
if.- if (h!=null) { + if (h != null) {
177-181: Consider adding validation for the maxDays value.The method accepts any integer value from the header without validation. Consider adding bounds checking to ensure the value is positive and reasonable.
private void honorMaxDaysHeader(HttpResponse response) { Header h = response.getFirstHeader("X-Max-Days"); if (h != null) { try { int headerMaxDays = Integer.parseInt(h.getValue()); if (headerMaxDays > 0) { maxDays = headerMaxDays; logger.debug("Max days overridden to {} from server header", maxDays); } else { logger.warn("Ignoring non-positive X-Max-Days header value: {}", headerMaxDays); } } catch (NumberFormatException e) { logger.warn("Invalid X-Max-Days header value: {}", h.getValue(), e); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
launchable/jar/exe_deploy.jaris excluded by!**/*.jar
📒 Files selected for processing (1)
src/main/java/com/launchableinc/ingest/commits/CommitGraphCollector.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.11)
- GitHub Check: build (windows-latest, 3.10)
- GitHub Check: build (windows-latest, 3.9)
- GitHub Check: build (windows-latest, 3.8)
- GitHub Check: build (windows-latest, 3.7)
- GitHub Check: build (windows-latest, 3.6)
- GitHub Check: build (ubuntu-20.04, 3.12)
- GitHub Check: build (ubuntu-20.04, 3.11)
- GitHub Check: build (ubuntu-20.04, 3.10)
- GitHub Check: build (ubuntu-20.04, 3.9)
- GitHub Check: build (ubuntu-20.04, 3.8)
- GitHub Check: build (ubuntu-20.04, 3.7)
- GitHub Check: build (ubuntu-20.04, 3.6)
🔇 Additional comments (2)
src/main/java/com/launchableinc/ingest/commits/CommitGraphCollector.java (2)
127-129: Good refactoring to enable header inspection.This change effectively stores the HTTP response in a variable before passing it to
getAdvertisedRefs, enabling the inspection of response headers for the new max days override functionality.
343-343:Details
❓ Verification inconclusive
Verify impact with updated maxDays value.
This line uses the
maxDaysvalue that can now be modified by the server through the "X-Max-Days" header. The override could significantly change the commit filtering behavior. Ensure this is the intended behavior.
🏁 Script executed:
#!/bin/bash # Search for other places where maxDays is used to confirm that overriding it centrally is appropriate rg "maxDays" --type javaLength of output: 709
Verify the Impact of the Updated
maxDaysOverrideThe code now allows the
maxDaysvalue to be modified via the "X-Max-Days" header inCommitGraphCollector.javaand is passed from a default (30 days) inCommitIngester.java. This change affects the commit filtering logic—in particular, the filter uses the expression:CommitTimeRevFilter.after(System.currentTimeMillis() - TimeUnit.DAYS.toMillis(maxDays))Please ensure that:
- The override of
maxDaysvia the HTTP header is an intentional design choice.- A non-default
maxDaysvalue (supplied by the header) does not inadvertently broaden or restrict the commit filtering beyond what is expected.- The interactions between
CommitIngester.javaandCommitGraphCollector.javaregardingmaxDaysremain consistent and clear from a maintenance perspective.
| /** | ||
| * When a user incorrectly configures shallow clone, the incremental nature of commit collection | ||
| * makes it really hard for us and users to collaboratively reset and repopulate the commit data. | ||
| * This server-side override mechanism makes it easier. | ||
| */ | ||
| private void honorMaxDaysHeader(HttpResponse response) { | ||
| Header h = response.getFirstHeader("X-Max-Days"); | ||
| if (h!=null) { | ||
| maxDays = Integer.parseInt(h.getValue()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Integer parsing.
The method correctly extracts and processes the "X-Max-Days" header, but lacks error handling if the header value isn't a valid integer. This could cause runtime exceptions if the server provides a non-numeric value.
private void honorMaxDaysHeader(HttpResponse response) {
Header h = response.getFirstHeader("X-Max-Days");
if (h!=null) {
- maxDays = Integer.parseInt(h.getValue());
+ try {
+ maxDays = Integer.parseInt(h.getValue());
+ logger.debug("Max days overridden to {} from server header", maxDays);
+ } catch (NumberFormatException e) {
+ logger.warn("Invalid X-Max-Days header value: {}", h.getValue(), e);
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * When a user incorrectly configures shallow clone, the incremental nature of commit collection | |
| * makes it really hard for us and users to collaboratively reset and repopulate the commit data. | |
| * This server-side override mechanism makes it easier. | |
| */ | |
| private void honorMaxDaysHeader(HttpResponse response) { | |
| Header h = response.getFirstHeader("X-Max-Days"); | |
| if (h!=null) { | |
| maxDays = Integer.parseInt(h.getValue()); | |
| } | |
| } | |
| /** | |
| * When a user incorrectly configures shallow clone, the incremental nature of commit collection | |
| * makes it really hard for us and users to collaboratively reset and repopulate the commit data. | |
| * This server-side override mechanism makes it easier. | |
| */ | |
| private void honorMaxDaysHeader(HttpResponse response) { | |
| Header h = response.getFirstHeader("X-Max-Days"); | |
| if (h!=null) { | |
| try { | |
| maxDays = Integer.parseInt(h.getValue()); | |
| logger.debug("Max days overridden to {} from server header", maxDays); | |
| } catch (NumberFormatException e) { | |
| logger.warn("Invalid X-Max-Days header value: {}", h.getValue(), e); | |
| } | |
| } | |
| } |
Making the future commit re-collection easier by overriding the max days from the server side.
Summary by CodeRabbit