-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-15196 - Improve Content Viewer resolution based on MIME Type #10686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
exceptionfactory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for proposing this improvement @pvillard31.
On initial review, I'm concerned about introducing a new REST API method for this purpose, as it raises other questions about how Content Viewers are registered. It may be better to consider an approach that allows a Content Viewer to register for a MIME Type pattern, versus a specific type.
The Spring Web framework also has a powerful Media Type class that supports some of the matching against vendor extended types, which may be worth considering for implementation.
I plan to take a closer look in the near future,
|
I explored different approaches and it felt like the best approach with minimal changes while having all of this logic in a central place and reduce specific logic on the frontend side. I didn't look into built-in Spring capabilities, I can have a look at what could be the option there. But I think some API changes are required in any case, either with this new one or by changing the existing one but not sure that is desirable. |
|
Thanks for the reply. My main initial concern is the new REST API method, as it adds another round trip for something that can be selected, although the resolution could be helpful in some scenarios. |
|
Yeah, I do see a potential approach when content viewers are registered instead and have a pattern there. Let me give this a try. |
|
Well, I don't see any good option in the end to have all of the logic on the backend side. The simplest approach would be to add the patterns directly into the file https://github.com/apache/nifi/blob/main/nifi-extension-bundles/nifi-standard-bundle/nifi-standard-content-viewer/src/main/webapp/META-INF/nifi-content-viewer and then do the pattern matching on the frontend side. I guess that could be OK but we are back at the discussion in the comments on the issue. |
|
I pushed a commit to switch to the very simple approach as a way to discuss both approaches. |
exceptionfactory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the alternative approach @pvillard31.
I will take a closer look at options, but it does seem like something along the simplified compatible check is a better option.
At a high level, a MIME Type such as application/vnd.something+json is fundamentally application/json, and that holds true of substituting anything for vnd.something with the + character. From that perspective, if a Content Viewer indicates support for application/json, it automatically supports any extension and should not necessarily need to include a pattern for matching. As you have in the simplified approach, this does put more responsibility on the frontend viewer to implement the compatibility checking, but that seems reasonable given the direct equivalence of extended types.
I would look for more input from others on the frontend side before getting too far, but this does help move the discussion forward to something tangible.
Summary
NIFI-15196 - Improve Content Viewer resolution based on MIME Type
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation