Combine query with request body parameters

[Crydust]

This way the return value of org.htmlunit.WebRequest.getParameters will better resemble that of a servlet api.

Fixes spring-projects/spring-framework#33249

[rbri]

@Crydust thanks for noting this. Will have a look - but i fear i need some coffee to check what is going on here and if we can fix this in a spring-compatible way. So please give me some time to check that.

[Crydust]

@rbri Thank you for the super fast reply. I've addressed the checkstyle issues. Let me know if you'd like more clarification or more unittests.

I've written a minimal testcase with spring boot here:
https://github.com/Crydust/testcasehtmlunit

[rbri]

@Crydust looks like your are right ;-)

The contract of the method from springs point of view

Some time ago, we had a long back and forth under spring-projects/spring-framework#28240 on a similar case. As a result of that HtmlUnit exposed WebRequest.getParameters() and we simplified our code in spring-projects/spring-framework@dd1e6b9, and that includes not having to parse the query string any more.

And from the javadoc itself

Similar to the servlet api this will work depending on the request type and check the url parameters and the body.

There is a lot more missing - look at the last commit.

The current plan is to rewrite the WebRequest2Test into a parameterized test - to build a test suite that handles all cases. @Crydust What do you think?

[rbri]

@Crydust have written the parameterized test... and now we reach the tricky part.

the Servlet - getParameters() method only returns the URL parameters for multipart requests. This differs from the expectations in your test case getParametersFromQueryAndUrlEncodedBodyPostWhenEncodingTypeIsMultipart().

You can look at the similar test case with the same name in class WebRequest2Test. Making the implementations of getParameters() closer to the servlet one will break this.

Do you have an idea, if this is ok from a spring point of view?

[rbri]

@Crydust have done all the adjustments to make getParameters() compatible with the output of the servlet method. You can try with the latest snapshot build 4.5.0-SNAPSHOT

[Crydust]

@rbri The build 4.5.0-SNAPSHOT fixes my problem, but unforunately introduces a new problem. WebRequest.normalize removes "duplicate" values. In my opinion this is should be reverted. One usecase it breaks is when a form contains multiple checkboxes with the same name.

I've updated the minimal testcase to use htmlunit 4.5.0-SNAPSHOT.
The previously failing tests are now green.
I've added two failing tests.
https://github.com/Crydust/testcasehtmlunit

[rbri]

WebRequest.normalize removes "duplicate" values. In my opinion this is should be reverted. One usecase it breaks is when a form contains multiple checkboxes with the same name.

@Crydust verry good point, but i checked with the servlet API - will review my tests and check again

[rbri]

@Crydust
Ok, now it gets more complicated - the implementation is correct - at least it returns exactly the same values as the getParameters() method from a real servlet.

To get the values in a real servlet, you have to call getParameterValues(string). Should i add support for this method also?

[rbri]

@Crydust sorry - my last answer wars total sch....

Let me check again... sorry

[rbri]

@Crydust ok, you are totally right, my test code was crap and is now fixed. Will provide a new snapshot later today - maybe i can improve the jdoc also

[Crydust]

You're a bit too fast for me to follow. I had just begun writing a pull request. I have used HttpServletRequest.getParameterNames and HttpServletRequest.getParameterValues. Your solution will probably be near identical to what I wrote.

[rbri]

sorry - hopefully we are now done with this. A new snapshot build is out.

@Crydust Thanks a lot for checking and your PR and....

[rbri]

@Crydust what do you think about this

the Servlet - getParameters() method only returns the URL parameters for multipart requests. This differs from the expectations in your test case getParametersFromQueryAndUrlEncodedBodyPostWhenEncodingTypeIsMultipart().

You can look at the similar test case with the same name in class WebRequest2Test. Making the implementations of getParameters() closer to the servlet one will break this.

Do you have an idea, if this is ok from a spring point of view?

[Crydust]

@rbri it seems like spring parses the "multipart/form-data" body and includes it's parameters.
Oddly it includes them BEFORE including the parameters from the query.
The new implementation fails to upload a file. (the only reason to use "multipart/form-data")

I've updated the minimal testcase to use the newer htmlunit 4.5.0-SNAPSHOT.
The previously failing tests are now green.
I've added three failing tests and one of them will attempt to upload a file.
https://github.com/Crydust/testcasehtmlunit

[rbri]

@Crydust ok, will update my impl and the tests - glad to see me feeling that this is a longer story was correct

[rbri]

@Crydust snapshot is updated, all your tests passing (at least here).

Can you please add tests for other methods than post for multipart requests. I'm not sure what spring requires in this cases.

[Crydust]

I've started working on more tests for other methods, but it's taking a bi tlonger than anticipated. I'll let you know when it's ready.

[Crydust]

I've updated the minimal testcase to use the newer htmlunit 4.5.0-SNAPSHOT.

I have previously made a mistake in test 8.
When running this test with HtmlUnit as browser it was "hidden, button, query".
When submitting this form with firefox or chrome the output is "query, hidden, button".
Conclusion: valuesOfX should be "query, hidden, button" instead of "hidden, button, query".

I added tests for PUT, DELETE, PATCH and OPTIONS.
The PATCH method is weird, I had to "fake it", which ... seems to be what most people do?
I couldn't figure out how to do TRACE.

Some of the tests fail.
https://github.com/Crydust/testcasehtmlunit

[rbri]

@Crydust thanks a lot, will have a look later and try to fix

[rbri]

@Crydust ok, made some progress but still 4 tests failing - will investigate this further tomorrow

[rbri]

@Crydust can you please validate the asserts with a real browser for this tests

the path tests are failing because of the order of the parameters and the url params - we have changed the order to make test 8 passing and now this fails....

[Crydust]

As this pull requestis closed I created a new one.
see #851