TREQ-21: Throw FileNotFoundException when resource is not found #2

SourcePond · 2017-06-08T17:40:01Z

Check on construction time whether the URL points to a local resource
Always throw a FileNotFoundException when the URL points to a local resource and url.openConnection throws an IOException
Added OSGi tests

SourcePond · 2017-06-08T17:41:00Z

This pull-request replaces #1

michaelsembwever

Thanks @SourcePond, the PR looks awesome.

I've added some trivial comments regarding the project's code style and so on…

Otherwise I need some time to check out the tests, as you're added new testing framework/libraries…

My other question was that you were making remote url protocols pluggable, at least in how we detect which protocols are local or not…
What else is required to add a protocol to work with url.openConnecton()?

I'm thinking that if REMOTE_PROTOCOLS can be extended by a system property, then it should be documented, including an example use-case or what else needs to be done for additional protocols.

michaelsembwever · 2017-06-13T02:30:31Z

tiles-request-api/pom.xml

+        <url.version>2.5.2</url.version>
+        <felix.version>5.6.4</felix.version>
+        <slf4j-api.version>1.7.25</slf4j-api.version>
+        <logback.version>1.2.2</logback.version>


Trivial:
We don't specify dependency versions by maven properties in the tiles projects.
Please put these in with the dependency elements.

michaelsembwever · 2017-06-13T02:33:38Z

tiles-request-api/src/main/java/org/apache/tiles/request/locale/URLApplicationResource.java

+import java.net.*;
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;


Trivial:
the import order was correct how it was. the project puts java.* imports first, and doesn't wildcard imports.

michaelsembwever · 2017-06-13T02:35:58Z

tiles-request-api/src/main/java/org/apache/tiles/request/locale/URLApplicationResource.java

+     * {@link IOException} will be thrown directly. If a url has a local protocol, then any {@link IOException}
+     * will be caught and transformed into a {@link FileNotFoundException}.
+     */
+    static final String REMOTE_PROTOCOLS_PROPERTY = "tiles.remoteProtocols";


Trivial:
according to Java's declaration order, this should appear before LOG is declared.

michaelsembwever · 2017-06-13T02:38:22Z

tiles-request-api/src/main/java/org/apache/tiles/request/locale/URLApplicationResource.java

+        String protocolsProp = getProperty(REMOTE_PROTOCOLS_PROPERTY);
+        if (protocolsProp != null) {
+            for (String protocol : protocolsProp.split(";")) {
+                remoteProtocols.add(protocol.trim());


this is adding a feature to the code. it will need a bit more documentation, and be a bit clearer in the issue and commit message, as currently it's titled as a bug fix.

perfect. thanks @SourcePond

michaelsembwever · 2017-06-13T02:39:48Z

tiles-request-api/src/main/java/org/apache/tiles/request/locale/URLApplicationResource.java

    /** the URL where the contents can be found. */
    private URL url;
    /** if the URL matches a file, this is the file. */
    private File file;
+    /** if the URL points to a local resource */
+    private boolean local;


i think we can make these fields final?

SourcePond · 2017-06-13T08:26:51Z

I added some documentation which should also explain how an additional remote protocol can be added to the system. Additionally, I enhanced the task a bit so it should explain the problem in more detail.

michaelsembwever · 2017-06-13T08:54:16Z

I added some documentation which should also explain how an additional remote protocol can be added to the system. Additionally, I enhanced the task a bit so it should explain the problem in more detail.

Thanks @SourcePond. Very much appreciated! Especially the response time :-)

michaelsembwever · 2017-06-13T09:23:52Z

tiles-request-api/src/test/java/org/apache/tiles/request/locale/URLApplicationResourceTest.java

-import java.net.URISyntaxException;
-import java.net.URL;
+import java.lang.reflect.Field;
+import java.net.*;


trivial: no wildcards please.

I'm sorry, I adjusted my IDE settings and it should not happen again now ;-)

michaelsembwever · 2017-06-18T11:57:46Z

tiles-request-api/src/test/java/org/apache/tiles/request/locale/URLApplicationResourceTest.java

+        try {
+            resource.getInputStream();
+        } catch (FileNotFoundException e) {
+            // expected


if this is expected, then shouldn't the method be annotated @Test(expected=FileNotFoundException.class) ?

michaelsembwever · 2017-06-18T11:59:00Z

tiles-request-api/src/test/java/org/apache/tiles/request/locale/URLApplicationResourceTest.java

+        } catch (FileNotFoundException e) {
+            fail("FileNotFoundException not allowed here");
+        } catch (IOException e) {
+            assertEquals(EXPECTED_MESSAGE, e.getMessage());


same comment here. shouldn't we also rethrow, and make it @Test(expected=IOException.class)

In my opinion, we should catch the IOException here, and, should assert that it was actually thrown by the TestUrlStreamHandler. This should make sure that the test does not accidentally pass even when an unexpected IOException occurs.

michaelsembwever · 2017-06-18T11:59:49Z

...equest-api/src/test/java/org/apache/tiles/request/osgi/URLBundleApplicationResourceTest.java

+            resource.getLastModified();
+            fail("Exception expected");
+        } catch (FileNotFoundException e) {
+            // expected


same comment about expected exceptions.

michaelsembwever · 2017-06-18T11:59:55Z

...equest-api/src/test/java/org/apache/tiles/request/osgi/URLBundleApplicationResourceTest.java

+            resource.getInputStream();
+            fail("Exception expected");
+        } catch (FileNotFoundException e) {
+            // expected


same comment about expected exceptions.

michaelsembwever · 2017-06-18T12:00:48Z

i've finished my review and apart from the comments about @Test(expected=…) this is 👍 from me.

michaelsembwever · 2017-06-19T06:33:03Z

@SourcePond i'm happy to merge this. thanks for the contribution!

Before I merge it, would you mind squashing the commits to one commit, writing up a summarised message (that includes a reference to this PR)?

- Separate between local and remote URL protocols, check on construction time whether the URL points to a local resource - Always throw a FileNotFoundException when the URL points to a local resource and url.openConnection throws an IOException - Added system property 'tiles.remoteProtocols' which can be used to specify additional remote protocols - Added OSGi tests including PAX-EXAM dependencies - PR: apache#2

SourcePond · 2017-06-19T07:28:05Z

I squashed all changes into 1 commit, I'm glad that I could help

michaelsembwever · 2017-06-20T00:04:14Z

Has been committed upstream.

TREQ-21: Convert IOException to FileNotFoundException on local protocols - Separate between local and remote URL protocols, check on construction time whether the URL points to a local resource - Always throw a FileNotFoundException when the URL points to a local resource and url.openConnection throws an IOException - Added system property 'tiles.remoteProtocols' which can be used to specify additional remote protocols - Added OSGi tests including PAX-EXAM dependencies references: - pull request: #2 - ticket: https://issues.apache.org/jira/browse/TREQ-21 Contributed by Roland Hauser ( https://github.com/SourcePond ) This closes #2 git-svn-id: https://svn.apache.org/repos/asf/tiles/request/trunk@1799291 13f79535-47bb-0310-9956-ffa450edef68

michaelsembwever suggested changes Jun 13, 2017

View reviewed changes

michaelsembwever reviewed Jun 13, 2017

View reviewed changes

michaelsembwever reviewed Jun 18, 2017

View reviewed changes

SourcePond force-pushed the TREQ_1_0_X branch from 30ae749 to c195af3 Compare June 19, 2017 07:25

asfgit closed this in 586d164 Jun 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TREQ-21: Throw FileNotFoundException when resource is not found #2

TREQ-21: Throw FileNotFoundException when resource is not found #2

SourcePond commented Jun 8, 2017

SourcePond commented Jun 8, 2017

michaelsembwever left a comment

michaelsembwever Jun 13, 2017

michaelsembwever Jun 13, 2017

michaelsembwever Jun 13, 2017

michaelsembwever Jun 13, 2017

michaelsembwever Jun 13, 2017

michaelsembwever Jun 13, 2017

michaelsembwever Jun 13, 2017

SourcePond commented Jun 13, 2017 •

edited

Loading

michaelsembwever commented Jun 13, 2017

michaelsembwever Jun 13, 2017

SourcePond Jun 13, 2017

michaelsembwever Jun 18, 2017

michaelsembwever Jun 18, 2017

SourcePond Jun 19, 2017

michaelsembwever Jun 18, 2017

michaelsembwever Jun 18, 2017

michaelsembwever commented Jun 18, 2017

michaelsembwever commented Jun 19, 2017

SourcePond commented Jun 19, 2017 •

edited

Loading

michaelsembwever commented Jun 20, 2017

TREQ-21: Throw FileNotFoundException when resource is not found #2

TREQ-21: Throw FileNotFoundException when resource is not found #2

Conversation

SourcePond commented Jun 8, 2017

SourcePond commented Jun 8, 2017

michaelsembwever left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SourcePond commented Jun 13, 2017 • edited Loading

michaelsembwever commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelsembwever commented Jun 18, 2017

michaelsembwever commented Jun 19, 2017

SourcePond commented Jun 19, 2017 • edited Loading

michaelsembwever commented Jun 20, 2017

SourcePond commented Jun 13, 2017 •

edited

Loading

SourcePond commented Jun 19, 2017 •

edited

Loading