You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by soenkeliebau <gi...@git.apache.org> on 2016/02/22 14:24:19 UTC
[GitHub] nifi pull request: NIFI-1539 - Add normalization of content type f...
GitHub user soenkeliebau opened a pull request:
https://github.com/apache/nifi/pull/242
NIFI-1539 - Add normalization of content type for content viewing
Pull request for [NIFI-1539](https://issues.apache.org/jira/browse/NIFI-1539)
Add code to ContentViewerController to strip content type of any trailing parameters and lowercase the type and subtype.
Added function to ViewableContent to enable retrieving the original value of the content type if needed.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/soenkeliebau/nifi NIFI-1539
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/242.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #242
----
commit 10d8227f443a82bfd644936866c18297f0ffb9e4
Author: Sönke Liebau <so...@opencore.com>
Date: 2016-02-22T07:25:42Z
NIFI-1539 - Add normalization of content type for content viewing
Add code to ContentViewerController to strip content type of any trailing parameters and lowercase the type and subtype.
Added function to ViewableContent to enable retrieving the original value of the content type if needed.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1539 - Add normalization of content type f...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/242#discussion_r54100675
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-content-viewer/src/main/java/org/apache/nifi/web/ContentViewerController.java ---
@@ -150,6 +150,7 @@ protected void doGet(final HttpServletRequest request, final HttpServletResponse
// buffer the content to support reseting in case we need to detect the content type or char encoding
try (final BufferedInputStream bis = new BufferedInputStream(downloadableContent.getContent());) {
final String mimeType;
+ final String normalizedMimeType;
// when standalone and we don't know the type is null as we were able to directly access the content bypassing the rest endpoint,
// when clustered and we don't know the type set to octet stream since the content was retrieved from the node's rest endpoint
--- End diff --
@soenkeliebau - I overlooked this the first time I reviewed because the GitHub diff view had it collapsed. I believe we need to modify the check on the next line that will allow Tika content type detection when the type from the DownableContent is null or if the value starts with ignore case "application/octet-stream".
This is a simple change I can make after applying your commit so I don't think we need to go through the process of re-submitting the PR, I just wanted to make sure you agree and we're on the same page.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1539 - Add normalization of content type f...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/242#discussion_r53682786
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-content-viewer/pom.xml ---
@@ -104,6 +104,14 @@
</excludes>
</configuration>
</plugin>
+ <plugin>
--- End diff --
NiFi requires Java 1.7. Is there any reason that is being overridden here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1539 - Add normalization of content type f...
Posted by soenkeliebau <gi...@git.apache.org>.
Github user soenkeliebau commented on a diff in the pull request:
https://github.com/apache/nifi/pull/242#discussion_r54103309
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-content-viewer/src/main/java/org/apache/nifi/web/ContentViewerController.java ---
@@ -150,6 +150,7 @@ protected void doGet(final HttpServletRequest request, final HttpServletResponse
// buffer the content to support reseting in case we need to detect the content type or char encoding
try (final BufferedInputStream bis = new BufferedInputStream(downloadableContent.getContent());) {
final String mimeType;
+ final String normalizedMimeType;
// when standalone and we don't know the type is null as we were able to directly access the content bypassing the rest endpoint,
// when clustered and we don't know the type set to octet stream since the content was retrieved from the node's rest endpoint
--- End diff --
@mcgilman - Do you mean that this part:
`downloadableContent.getType().equals(MediaType.OCTET_STREAM.toString())`
need the same transformation applied as normalizedMimeType further down, before comparing it to anything?
If so, then yes, I see what you mean and fully agree, please feel free to make the change as you see fit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1539 - Add normalization of content type f...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the pull request:
https://github.com/apache/nifi/pull/242#issuecomment-187358663
Overall, the changes look good. Just need to clarify the point about the Java version and verify it works now that NIFI-1546 is addressed.
Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1539 - Add normalization of content type f...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the pull request:
https://github.com/apache/nifi/pull/242#issuecomment-188831478
This has been merged into master. Thanks again for the contribution!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1539 - Add normalization of content type f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/242
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1539 - Add normalization of content type f...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the pull request:
https://github.com/apache/nifi/pull/242#issuecomment-188776138
Will be finishing this review today
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1539 - Add normalization of content type f...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/242#discussion_r54104580
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-content-viewer/src/main/java/org/apache/nifi/web/ContentViewerController.java ---
@@ -150,6 +150,7 @@ protected void doGet(final HttpServletRequest request, final HttpServletResponse
// buffer the content to support reseting in case we need to detect the content type or char encoding
try (final BufferedInputStream bis = new BufferedInputStream(downloadableContent.getContent());) {
final String mimeType;
+ final String normalizedMimeType;
// when standalone and we don't know the type is null as we were able to directly access the content bypassing the rest endpoint,
// when clustered and we don't know the type set to octet stream since the content was retrieved from the node's rest endpoint
--- End diff --
Yep, for complete clarity this is the additional commit I'll be including [1].
[1] https://github.com/mcgilman/nifi/commit/db02e18580a12b1321a9324bd0fb3d3354f9f7f5
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1539 - Add normalization of content type f...
Posted by soenkeliebau <gi...@git.apache.org>.
Github user soenkeliebau commented on a diff in the pull request:
https://github.com/apache/nifi/pull/242#discussion_r54104990
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-content-viewer/src/main/java/org/apache/nifi/web/ContentViewerController.java ---
@@ -150,6 +150,7 @@ protected void doGet(final HttpServletRequest request, final HttpServletResponse
// buffer the content to support reseting in case we need to detect the content type or char encoding
try (final BufferedInputStream bis = new BufferedInputStream(downloadableContent.getContent());) {
final String mimeType;
+ final String normalizedMimeType;
// when standalone and we don't know the type is null as we were able to directly access the content bypassing the rest endpoint,
// when clustered and we don't know the type set to octet stream since the content was retrieved from the node's rest endpoint
--- End diff --
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---