-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JBEAP-26402: handle badly formatted artifact cache #540
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @michpetrov. A couple of comments, plus could you port it to 1.1.x branch as well please?
@@ -205,6 +205,9 @@ private void init() throws IOException { | |||
final List<String> lines = Files.readAllLines(artifactLog); | |||
for (String line : lines) { | |||
final String[] splitLine = line.split(ArtifactCache.CACHE_LINE_SEPARATOR); | |||
if (splitLine.length < 3) { | |||
throw new MavenUniverseException("Bad record format"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this to the translations in ProsperoLogger with a bit more context please. Maybe something like "Bad artifact record format in the cache descriptor: %s"
Also I think just a base IOException would be better, it has nothing to do with MavenUniverse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I add this to the logger what about the IOE in the catch block? I'd assume we want the top message to be the same, i.e. "can't read cache".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah good point, I remember why it was using MavenUniverseException. It's the top level catch that should have a more descriptive error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spyrkob well what messages do you want me to change then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michpetrov the one in throw new IOException("Unable to read cached items.", e);
if you don't mind adding this to this PR
Path newFolder = temp.newFolder().toPath(); | ||
Files.createDirectories(newFolder.resolve(ArtifactCache.CACHE_FOLDER)); | ||
Files.writeString(newFolder.resolve(ArtifactCache.CACHE_FOLDER).resolve(ArtifactCache.CACHE_FILENAME),"badformat"); | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use following instead
assertThatThrownBy(() -> { ArtifactCache.getInstance(newFolder)})
.isInstanceOf(IOException.class)
.hasMessageContaining("Bad record format");
10e5755
to
71a6628
Compare
@spyrkob does this work? I'll port it to 1.1.x if there're no further requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @michpetrov that looks great!
71a6628
to
f602605
Compare
Issue: JBEAP-26402