Skip to content

normalization should not change KeyDataPair to NameValuePair - #533

Merged
rbri merged 2 commits into
HtmlUnit:masterfrom
thuri:master
Dec 21, 2022
Merged

normalization should not change KeyDataPair to NameValuePair#533
rbri merged 2 commits into
HtmlUnit:masterfrom
thuri:master

Conversation

@thuri

@thuri thuri commented Dec 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

@thuri thuri changed the title normalization should not change KeyDataPair to NameValuePair Fixes #532 normalization should not change KeyDataPair to NameValuePair Dec 20, 2022
rbri
rbri requested changes Dec 21, 2022
View reviewed changes

@rbri rbri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks reasonable for me, thanks for this.
Please add yourself as author to all changed files and maybe also to the pom (if you are not already in the list)

Comment thread src/main/java/com/gargoylesoftware/htmlunit/util/NameValuePair.java
Comment thread src/test/java/com/gargoylesoftware/htmlunit/WebRequestTest.java
@rbri

rbri commented Dec 21, 2022

Copy link
Copy Markdown
Member

Jenkins will have a look at this....

@rbri

rbri commented Dec 21, 2022

Copy link
Copy Markdown
Member

sorry we are still at java8 :-)

/var/lib/jenkins/workspace/htmlunit_PR-533/src/test/java/com/gargoylesoftware/htmlunit/WebRequestTest.java:[437,42] cannot find symbol
symbol:   method of(com.gargoylesoftware.htmlunit.util.NameValuePair,com.gargoylesoftware.htmlunit.util.KeyDataPair)
location: interface java.util.List

@thuri

thuri commented Dec 21, 2022

Copy link
Copy Markdown
Contributor Author

Yeah I've seen that but somehow eclipse didn't warn me when i used Java 11 features.
I corrected the problematic part. At least I hope so

rbri
rbri reviewed Dec 21, 2022
View reviewed changes
Comment thread src/main/java/com/gargoylesoftware/htmlunit/util/KeyDataPair.java Outdated
public KeyDataPair normalized() {
final KeyDataPair pair = new KeyDataPair(
this.getName(),
this.fileObject_ != null ? this.fileObject_ : new File(this.getValue()),

@rbri rbri Dec 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

new File(...) might do some normalization on the file name leading in the end to some new value -> Testcase?
maybe it is better to have a private constructor that allows to pass the values 1:1?

What do you think?

@rbri

rbri commented Dec 21, 2022

Copy link
Copy Markdown
Member

tests are looking good !

@thuri
private copy constructor for KeyDataPair
98368c2 
this should prevent manipulation of fields and make sure that
"normalized" instances are identical copies if the source object is
normalized already
@thuri

thuri commented Dec 21, 2022

Copy link
Copy Markdown
Contributor Author

@rbri please have a look at the additional commit. Perhaps that is what you had in mind ...

@rbri
rbri merged commit ecfbc91 into HtmlUnit:master Dec 21, 2022
@rbri

rbri commented Dec 21, 2022

Copy link
Copy Markdown
Member

i have nothing in mind ;-)
thanks a lot - will inform when the new snapshot is availble

and all the best for 2023

@thuri

thuri commented Dec 21, 2022

Copy link
Copy Markdown
Contributor Author

Thanks a lot and the all the best for you too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rbri rbri rbri requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RequestParameter normalization replaces KeyDataPair with NameValuePair

2 participants

@thuri @rbri