HADOOP-19079. Check class is an exception class before constructing an instance #6557
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Feb 16, 2024
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
yeah, I can see the need for this
| Mockito.when(conn.getResponseCode()).thenReturn( | ||
| HttpURLConnection.HTTP_BAD_REQUEST); | ||
| try { | ||
| HttpExceptionUtils.validateResponse(conn, HttpURLConnection.HTTP_CREATED); |
Contributor
There was a problem hiding this comment.
use LambdaTestUtils.intercept(), which is based off ScalaTest's version, includes test for error message text.
intercept(IOException.class,
"HTTP status [400], exception [java.lang.String], message [EX], URL [null]",
() -> HttpExceptionUtils.validateResponse(conn, HttpURLConnection.HTTP_CREATED);
Member
Author
There was a problem hiding this comment.
I can change this but I just want to highlight that this is a modified copy of existing tests in this test class and this is the code style in the existing tests.
Contributor
There was a problem hiding this comment.
old code, going near the test is the time to clean it up.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Member
Author
|
Member
Author
|
@steveloughran @slfan1989 is this something that could be considered for Hadoop 3.4.0 or 3.4.1? |
pjfanning
force-pushed
the
HADOOP-19079-class-check
branch
from
c44f222 to
b20a5d8
Compare
|
💔 -1 overall
This message was automatically generated. |
Member
Author
|
@steveloughran is this something that could be merged to trunk with a view to also merging into 3.4.1? |
steveloughran
requested changes
Mar 19, 2024
| Constructor constr = klass.getConstructor(String.class); | ||
| toThrow = (Exception) constr.newInstance(exMsg); | ||
| } catch (Exception ex) { | ||
| if (!Exception.class.isAssignableFrom(klass)) { |
Contributor
There was a problem hiding this comment.
use Preconditions.checkState() with error text including classname. if it ever does happen, we will want to debug
| Mockito.when(conn.getResponseCode()).thenReturn( | ||
| HttpURLConnection.HTTP_BAD_REQUEST); | ||
| try { | ||
| HttpExceptionUtils.validateResponse(conn, HttpURLConnection.HTTP_CREATED); |
Contributor
There was a problem hiding this comment.
old code, going near the test is the time to clean it up.
pjfanning
force-pushed
the
HADOOP-19079-class-check
branch
from
b20a5d8 to
65efd0d
Compare
|
🎊 +1 overall
This message was automatically generated. |
Member
Author
|
@steveloughran I made some changes based on your review. |
steveloughran
requested changes
Mar 28, 2024
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
like the changes, now just wondering how much of a check should be done in intercept() versus having some uses of assertExceptionContains() afterwards
ioe e = intercept(IOE,"400", () -> {...})
assertExceptionContains(e, "EX")
...ommon-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHttpExceptionUtils.java
Outdated
Show resolved
Hide resolved
pjfanning
force-pushed
the
HADOOP-19079-class-check
branch
from
65efd0d to
fa79af3
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
looks good, I am worried about overloaded methods though -we've had so many problems there in the past. the new method is good (one extra resilience change proposed) -but should we still use intercept() or give it a slightly different name.
we lifted the name from ScalaTest, if you've never played with that yet.
| throws Exception { | ||
| return intercept(clazz, | ||
| null, | ||
| (String) null, |
Contributor
There was a problem hiding this comment.
ooh, I worry if there are going to be link problems by anything not in our codebase using the method (gcs etc). Hope not.
| Assert.assertTrue(ex.getMessage().contains("" + | ||
| HttpURLConnection.HTTP_BAD_REQUEST)); | ||
| } | ||
| Collection<String> expectedValues = |
Contributor
There was a problem hiding this comment.
Arrays.asList() is easier
| ex = (E) e; | ||
| } | ||
| } | ||
| for (String contained : contains) { |
Contributor
There was a problem hiding this comment.
skip if contains==null
Member
Author
I am very familiar with Scala test. I code mainly in Scala. Junit has assertThrows though. Would that be a bit more Java friendly? |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| * can be determined. | ||
| * @see GenericTestUtils#assertExceptionContains(String, Throwable) | ||
| */ | ||
| public static <T, E extends Throwable> E intercept( |
Contributor
There was a problem hiding this comment.
I do think this is a good feature, it's just we need to give it a name which doesn't run any risk of causing compilation problems.
alternatively, in GenericTestUtils add a new method assertExceptionContainsAllStrings() taking a varargs list of strings.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Apr 10, 2024
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
code looking good now, one change left which is to use the java8 string concat stuff. commons-collections can also do this, but the JDK stuff works perfectly well
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/LambdaTestUtils.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
merged to trunk; cherrypicking to branch-3.4. branch-3.3 as well, perhaps |
steveloughran
pushed a commit
that referenced
this pull request
Apr 11, 2024
…y an exception before instantiation (#6557) Security hardening + Adds new interceptAndValidateMessageContains() method in LambdaTestUtils to verify a list of strings can all be found in the toString() value of a raised exception Contributed by PJ Fanning
asfgit
pushed a commit
that referenced
this pull request
Apr 11, 2024
…y an exception before instantiation (#6557) Security hardening + Adds new interceptAndValidateMessageContains() method in LambdaTestUtils to verify a list of strings can all be found in the toString() value of a raised exception Contributed by PJ Fanning
Contributor
one thing intercept does, which I haven't seen the others to, is include the toString() value of anything returned by the callable in the assertion. which lets you add tests that explicitly print their state on failures, rather than just "l-exp invoked didn't fail". |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
HADOOP-19079
How was this patch tested?
New test added
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?