Skip to content
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.

Sign up for GitHub

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

[JENKINS-73690] Avoid null Enum to throw an exception #577

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[JENKINS-73690] Avoid null Enum to throw an exception #577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b83c9f4
[JENKINS-73690] Avoid null Enum to throw an exception
Wadeck Aug 27, 2024
5bf1407
Apply spotless formatting
MarkEWaite Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions core/src/main/java/org/kohsuke/stapler/Stapler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1255,6 +1255,9 @@ public org.apache.commons.fileupload.FileItem convert(Class type, Object value)
private static final Converter ENUM_CONVERTER = new Converter() {
@Override
public Object convert(Class type, Object value) {
if (value == null) {
return null;
}
return Enum.valueOf(type, value.toString());
}
};
Expand Down
87 changes: 87 additions & 0 deletions core/src/test/java/org/kohsuke/stapler/DataBindingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@
import java.util.Collections;
import java.util.EnumSet;
import java.util.List;
import javax.servlet.ServletException;
import junit.framework.TestCase;
import net.sf.json.JSONArray;
import net.sf.json.JSONObject;
import org.jvnet.hudson.test.Issue;

/**
* @author Kohsuke Kawaguchi
Expand Down Expand Up @@ -430,4 +432,89 @@ public void testDerivedProperty() {
DerivedProperty r = bind("{items:[1,3,5]}", DerivedProperty.class);
assertEquals(Arrays.asList(1, 3, 5), r.getItems());
}

@Issue("JENKINS-73690")
public void testEnumCanBeNull() throws Exception {
MockRequest mr = new MockRequest() {
@Override
public String getContentType() {
return "text/html";
}
};
mr.getParameterMap().put("status", null);
RequestImpl req = new RequestImpl(new Stapler(), mr, Collections.emptyList(), null);
Object result = new Function.InstanceFunction(
getClass().getMethod("doAcceptEnum", StaplerRequest.class, Status.class))
.bindAndInvoke(this, req, null);
assertNull(result);
}

public void testEnumIsSupported() throws Exception {
MockRequest mr = new MockRequest() {
@Override
public String getContentType() {
return "text/html";
}
};
mr.getParameterMap().put("status", "Done");
RequestImpl req = new RequestImpl(new Stapler(), mr, Collections.emptyList(), null);
Object result = new Function.InstanceFunction(
getClass().getMethod("doAcceptEnum", StaplerRequest.class, Status.class))
.bindAndInvoke(this, req, null);
assertEquals(Status.Done, result);
}

public void testEnumUnknownIsThrowing() throws Exception {
MockRequest mr = new MockRequest() {
@Override
public String getContentType() {
return "text/html";
}
};
mr.getParameterMap().put("status", "Blabla");
RequestImpl req = new RequestImpl(new Stapler(), mr, Collections.emptyList(), null);
try {
new Function.InstanceFunction(getClass().getMethod("doAcceptEnum", StaplerRequest.class, Status.class))
.bindAndInvoke(this, req, null);
fail("Should throw an exception as the enum value is not recognized");
} catch (IllegalArgumentException e) {
assertEquals(
"No enum constant org.kohsuke.stapler.DataBindingTest.Status.Blabla",
e.getCause().getMessage());
}
}

// Just to ensure to keep support for required=true
@Issue("JENKINS-73690")
public void testEnumCannotBeNullIfRequired() throws Exception {
MockRequest mr = new MockRequest() {
@Override
public String getContentType() {
return "text/html";
}
};
mr.getParameterMap().put("status", null);
RequestImpl req = new RequestImpl(new Stapler(), mr, Collections.emptyList(), null);
try {
new Function.InstanceFunction(getClass().getMethod("doRequireEnum", StaplerRequest.class, Status.class))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use assertThrows for cleaner and less code in both these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the pattern used in that class file, for the testMismatchingTypes test case.

I usually prefer consistent style. Only occurrence of assertThrows is in Stapler2Test, compared to ~20 other occurrences of try/catch.

In a new project or if I was refactoring, yes I would have used assertThrows :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo you don't need to add harder to read code because existing code is hard to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depend on the perspective. If you look only at the diff, yes I agree, but if you look at the whole file, having different style make it harder. I expect to have more people reading the whole file compared to people reading only this PR/delta.

.bindAndInvoke(this, req, null);
fail("Should throw an exception as the enum value is not recognized");
} catch (ServletException e) {
assertEquals("Required Query parameter status is missing", e.getMessage());
}
}

enum Status {
Todo,
Progress,
Done
}

public Object doAcceptEnum(StaplerRequest req, @QueryParameter Status status) {
return status;
}

public Object doRequireEnum(StaplerRequest req, @QueryParameter(required = true) Status status) {
return status;
}
}
Loading