Properly encoded URIs use %20

[ajgassner]

should fix #5360

see https://stackoverflow.com/a/31612226/1296333

---

This change is 

[CLAassistant]

All committers have signed the CLA.

[caalador]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @ajgassner and @mehdi-vaadin)

---

flow-server/src/test/java/com/vaadin/flow/server/StreamResourceRegistryTest.java, line 145 at r2 (raw file):

        URI url = registration.getResourceUri();
        String suffix = URLEncoder.encode(resource.getName(),

This test is now wrong as the result should by specification be a%3Fb%3Dc+d%26e as its a query parameter a new test would be required that would have the resource "my file.png" which should become "my%20file.png".

This does raise the question that should a StreamResource accept query parameters in the name.

[ajgassner]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @caalador and @mehdi-vaadin)

---

flow-server/src/test/java/com/vaadin/flow/server/StreamResourceRegistryTest.java, line 145 at r2 (raw file):

Previously, caalador wrote…

This test is now wrong as the result should by specification be a%3Fb%3Dc+d%26e as its a query parameter a new test would be required that would have the resource "my file.png" which should become "my%20file.png".

This does raise the question that should a StreamResource accept query parameters in the name.

StreamResource doesn't accept/allow query parameters in the moment as the whole resource name gets URL encoded. Also the question mark gets url encoded, so it's not a marker for query params.

So IMHO this test is correct. "a?b=c d&e" represents a resource name, not an URL part. Imagine you have a file with filename "abc? test123.png". This should become "abc%3F%20test123.png".

[caalador]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @ajgassner and @mehdi-vaadin)

---

flow-server/src/test/java/com/vaadin/flow/server/StreamResourceRegistryTest.java, line 145 at r2 (raw file):

Previously, ajgassner (Alex Gassner) wrote…

StreamResource doesn't accept/allow query parameters in the moment as the whole resource name gets URL encoded. Also the question mark gets url encoded, so it's not a marker for query params.

So IMHO this test is correct. "a?b=c d&e" represents a resource name, not an URL part. Imagine you have a file with filename "abc? test123.png". This should become "abc%3F%20test123.png".

Windows for instance doesn't accept ? in a file name so it's a bit hard to accept.
It would be nice to have another name there instead.

But a test would be needed that checks that the original file containing + and image++.svg works.
Shouldn't really have anything to do with this as + should be encoded to %2B by URLEncoder and
image++.svg should become something like VAADIN/dynamic/resource/1/9bf0b824-3851-47a1-b16a-f68b8053ac94/image%2B%2B.svg

To actually fix the issue with I would expect a change also to StreamRequestHandler::getPathUri

[ajgassner]

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @ajgassner and @caalador)

---

flow-server/src/test/java/com/vaadin/flow/server/StreamResourceRegistryTest.java, line 145 at r2 (raw file):

Previously, caalador wrote…

Windows for instance doesn't accept ? in a file name so it's a bit hard to accept.
It would be nice to have another name there instead.

But a test would be needed that checks that the original file containing + and image++.svg works.
Shouldn't really have anything to do with this as + should be encoded to %2B by URLEncoder and
image++.svg should become something like VAADIN/dynamic/resource/1/9bf0b824-3851-47a1-b16a-f68b8053ac94/image%2B%2B.svg

To actually fix the issue with I would expect a change also to StreamRequestHandler::getPathUri

Done.

[caalador]

Reviewed 2 of 2 files at r3.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @ajgassner)

a discussion (no related file):
Could you add a download for a filename with + and or ++ in it to StreamResourceView and add a download test for it to StreamResourceIT

Also if possible a verification to StreamResourceHandlerTest would be good to have.

---

---

flow-server/src/main/java/com/vaadin/flow/server/communication/StreamRequestHandler.java, line 170 at r3 (raw file):

name = name.replace('+', ' ');

This seems like it might mess up a name with + in it.

[ajgassner]

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador)

a discussion (no related file):

Previously, caalador wrote…

Could you add a download for a filename with + and or ++ in it to StreamResourceView and add a download test for it to StreamResourceIT

Also if possible a verification to StreamResourceHandlerTest would be good to have.

I have implemented the IT. But I'm unable to run it locally. Are there docs how to get the UI tests up and running with IntelliJ?

---

---

flow-server/src/main/java/com/vaadin/flow/server/communication/StreamRequestHandler.java, line 170 at r3 (raw file):

Previously, caalador wrote…

name = name.replace('+', ' ');

This seems like it might mess up a name with + in it.

I don't get why. Example input "a+b" becomes "a b". Then the URLEncoder produces "a+b" again, because of the space. Then the + gets replaced, results in "a%20b".

[ajgassner]

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador)

a discussion (no related file):

Previously, ajgassner (Alex Gassner) wrote…

I have implemented the IT. But I'm unable to run it locally. Are there docs how to get the UI tests up and running with IntelliJ?

Ah I see, I need a ro license...

---

[ajgassner]

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador)

a discussion (no related file):

Previously, ajgassner (Alex Gassner) wrote…

Ah I see, I need a ro license...

@caalador The test is failing, can you please fix it? I'm unable to run them locally. That's why it's pretty hard for me to find the cause. It's annoying to wait for TeamCity after each change. I think the problem is somewhere in the test code itself. Selenium decodes the url or something like that.

---

[caalador]

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @ajgassner and @caalador)

a discussion (no related file):

Previously, ajgassner (Alex Gassner) wrote…

@caalador The test is failing, can you please fix it? I'm unable to run them locally. That's why it's pretty hard for me to find the cause. It's annoying to wait for TeamCity after each change. I think the problem is somewhere in the test code itself. Selenium decodes the url or something like that.

@ajgassner I'll have a look.

---

[caalador]

This patch should fix the PR I couldn't push so I need to as if you could put in the patch.
0001-Do-not-change-resource-name.patch.txt

[caalador]

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @ajgassner)

---

flow-server/src/main/java/com/vaadin/flow/server/communication/StreamRequestHandler.java, line 170 at r3 (raw file):

Previously, ajgassner (Alex Gassner) wrote…

I don't get why. Example input "a+b" becomes "a b". Then the URLEncoder produces "a+b" again, because of the space. Then the + gets replaced, results in "a%20b".

It's because now you specifically encode the spaces as %20 and the actual plus that has been encoded as %2B will be decoded at this time making us replace the + that was not a space with and thus loosing the actual + as it is now suddenly handled as a space.

[ajgassner]

@caalador you are now contributor of git@github.com:ajgassner/flow.git Please push your changes to master.

[mehdi-vaadin]

@ajgassner thanks for your PR. Is the proposed change supposed to fix the following piece of code too?

Anchor downloadAnchor = new Anchor(new StreamResource("a+b.txt", (stream, session) -> {
    stream.write("Something".getBytes());
}), "download");

add(downloadAnchor);

I tried it and got this error:
HTTP ERROR 404 Resource is not found for path=VAADIN/dynamic/resource/2/50719cd4-6a52-4240-a9f9-dbcae9618f49/a+b.txt

[ajgassner]

@mehdi-vaadin Yes it is. Mhm the generated URI should end with a%20b.txt

[mehdi-vaadin]

The whole error page:

[caalador]

@mehdi did you test with my patch in?

Made changes myself

[mehdi-vaadin]

Yes, I got your patch.

[mehdi-vaadin]

Reviewed 1 of 2 files at r3, 3 of 4 files at r5.
Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @ajgassner and @mehdi-vaadin)

---

flow-server/src/test/java/com/vaadin/flow/server/StreamResourceRegistryTest.java, line 158 at r6 (raw file):

        URI uri = registration.getResourceUri();
        Assert.assertTrue("Resource URI is not properly encoded", uri.toString().endsWith(suffix));

Trivial: this file needs reformatting based on Floe conventions.

---

flow-server/src/test/java/com/vaadin/flow/server/communication/StreamRequestHandlerTest.java, line 34 at r6 (raw file):

StreamRequestHandlerTest

Trivial: the formatting of this class is not based on Flow conventions.

---

flow-server/src/test/java/com/vaadin/flow/server/communication/StreamRequestHandlerTest.java, line 73 at r6 (raw file):

Plusses

Typo: Pluses
Same in other places in this file.

[caalador]

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @ajgassner and @mehdi-vaadin)

---

flow-server/src/test/java/com/vaadin/flow/server/communication/StreamRequestHandlerTest.java, line 34 at r6 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…

StreamRequestHandlerTest

Trivial: the formatting of this class is not based on Flow conventions.

It's exactly the same as I use for all other PRs in flow. Only difference I can see after mvn formatter:format are a couple different line breaks.

---

flow-server/src/test/java/com/vaadin/flow/server/communication/StreamRequestHandlerTest.java, line 73 at r6 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…

Plusses

Typo: Pluses
Same in other places in this file.

done

[caalador]

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador and @mehdi-vaadin)

---

flow-server/src/test/java/com/vaadin/flow/server/StreamResourceRegistryTest.java, line 158 at r6 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…

Trivial: this file needs reformatting based on Floe conventions.

done

[mehdi-vaadin]

Reviewed 2 of 2 files at r7.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained

---

flow-server/src/test/java/com/vaadin/flow/server/communication/StreamRequestHandlerTest.java, line 34 at r6 (raw file):

Previously, caalador wrote…

It's exactly the same as I use for all other PRs in flow. Only difference I can see after mvn formatter:format are a couple different line breaks.

You're right. Sorry. My bad. :)