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.

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

Support empty file uploads with HtmlUnit and MockMvc #26799

Closed
tloist opened this issue Apr 13, 2021 · 4 comments
Closed

Support empty file uploads with HtmlUnit and MockMvc #26799

tloist opened this issue Apr 13, 2021 · 4 comments
Assignees
Labels
in: test Issues in the test module type: bug A general bug
Milestone

Comments

@tloist
Copy link

tloist commented Apr 13, 2021

When one uses HtmlUnit as a Web Client for tests, the HtmlUnitRequestBuilder can't cope with empty file input fields. It just tries to access the field instance and throws a NullPointerException.

HtmlUnit produces an KeyValuePair with a null file (so the file variable is explicitly set to null). However the current implementation in Spring doesn't cope well with that field being null, which results in a NullPointerException being thrown.

I guess an empty file input field should just be ignored, so maybe the best way to deal with it would be something along the lines of:

if (pair.getFile() == null) continue;

because I don't think we should drop to the default handling of request.addParameter(param.getName(), param.getValue()); a few lines later.

For reference, I think the problem was introduced in this pull request (1 year)
where as HtmlUnit has this behavior probably longer (the last change on the line was 4 years before). Which is why I think it makes probably more sense to fix it here.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 13, 2021
@sbrannen sbrannen added the in: test Issues in the test module label Apr 14, 2021
@sbrannen
Copy link
Member

I guess an empty file input field should just be ignored,

Perhaps instead of ignoring it we should fall back to the default handling and add the parameter's name and value to the request (as if it were not a KeyDataPair).

@rstoyanchev, thoughts?

@sbrannen sbrannen added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 14, 2021
@sbrannen sbrannen added this to the 5.3.7 milestone Apr 14, 2021
@sbrannen sbrannen self-assigned this Apr 14, 2021
@sbrannen
Copy link
Member

sbrannen commented May 5, 2021

This has been fixed in main (in 959e6d1) and will be available in the upcoming 5.3.7 snapshot builds.

@tloist, it would be great if you could try this out with a snapshot build and let us know if the fix works for your use cases.

@tloist
Copy link
Author

tloist commented May 6, 2021

Yes, we will try to. It's actual not my nor my projects setup so I am acting more or less as a proxy.
I just helped find the root cause and wanted to communicate it back.

I am gently, well, pushing for it, but I am not into their timeline so I can't exactly say when to expect some results.
I am however actively advocating my help and understand why it would be great to have actual feedback on the issue.
Please don't think I am not interested anymore - it's just a bit more complicated as I'd like it to be but I am on it.

sbrannen added a commit to sbrannen/spring-framework that referenced this issue May 7, 2021
This commit revises the fix submitted in 959e6d1 by ensuring that
empty file input is converted to a MockPart with the supplied name, an
empty filename, "application/octet-stream" as the content type, and
empty content.

This aligns with the behavior of Servlet containers, as tested with the
interaction between Firefox and a standard Servlet running in a Jetty
Servlet container.

Closes spring-projectsgh-26799
@sbrannen
Copy link
Member

sbrannen commented May 7, 2021

Please don't think I am not interested anymore - it's just a bit more complicated as I'd like it to be but I am on it.

Understood!


Please note that I have slightly revised the fix in d79e33b.

@sbrannen sbrannen added the for: backport-to-5.2.x Marks an issue as a candidate for backport to 5.2.x label May 7, 2021
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.2.x Marks an issue as a candidate for backport to 5.2.x labels May 7, 2021
@sbrannen sbrannen removed the status: backported An issue that has been backported to maintenance branches label May 7, 2021
@sbrannen sbrannen changed the title HtmlUnitRequestBuilder can't cope with empty file input (NPE) Support empty file uploads with HtmlUnit and MockMvc May 7, 2021
Zoran0104 pushed a commit to Zoran0104/spring-framework that referenced this issue Aug 20, 2021
Prior to this commit, when using HtmlUnit with empty file input,
MockMvc's HtmlUnitRequestBuilder would throw a NullPointerException
when attempting to create a MockPart based on the null File.

This commit ensures that empty file input is converted to a MockPart
with a valid name but with a null filename, a null content type, and
empty content.

Closes spring-projectsgh-26799
Zoran0104 pushed a commit to Zoran0104/spring-framework that referenced this issue Aug 20, 2021
This commit revises the fix submitted in 959e6d1 by ensuring that
empty file input is converted to a MockPart with the supplied name, an
empty filename, "application/octet-stream" as the content type, and
empty content.

This aligns with the behavior of Servlet containers, as tested with the
interaction between Firefox and a standard Servlet running in a Jetty
Servlet container.

Closes spring-projectsgh-26799
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
Prior to this commit, when using HtmlUnit with empty file input,
MockMvc's HtmlUnitRequestBuilder would throw a NullPointerException
when attempting to create a MockPart based on the null File.

This commit ensures that empty file input is converted to a MockPart
with a valid name but with a null filename, a null content type, and
empty content.

Closes spring-projectsgh-26799
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
This commit revises the fix submitted in 959e6d1 by ensuring that
empty file input is converted to a MockPart with the supplied name, an
empty filename, "application/octet-stream" as the content type, and
empty content.

This aligns with the behavior of Servlet containers, as tested with the
interaction between Firefox and a standard Servlet running in a Jetty
Servlet container.

Closes spring-projectsgh-26799
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants