[MGNLIMG-163] Imaging always strips extension from the name of the image when searching for matching nodes Created: 03/Jun/15  Updated: 15/Jul/15  Resolved: 14/Jul/15

Status: Closed
Project: Imaging
Component/s: None
Affects Version/s: 3.1.3
Fix Version/s: 3.1.4, 3.2.1

Type: Bug Priority: Critical
Reporter: Jan Schulte Assignee: Antonín Juran
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: 0d
Time Spent: 0.25d
Original Estimate: Not Specified

Attachments: PNG File 5.3.7 Image upload.png     PNG File 5.3.8 Image upload.png    
Issue Links:
Relates
relates to MGNLDAM-589 Incoherence on uploading: Upload asse... Closed
causality
caused by MGNLDAM-536 Files with whitespace in filename are... Closed
dependency
depends upon MGNLDAM-600 Increase dependency on Imaging 3.1.4. Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

The behaviour of the dam app image upload changed from 5.3.7 to 5.3.8.
Given that we have a photo.jpg: The node created in 5.3.7 after upload is named "photo" in jcr.
In 5.3.8 it is named "photo.jpg" in jcr after upload.
This caused problems in custom image operation chains as images uploaded in 5.3.8 must be called with ../photo.jpg.jpg



 Comments   
Comment by Antonín Juran [ 18/Jun/15 ]

After integration need to increase dependency in DAM 2.0.9 on Imaging 3.1.4.

Comment by Jan Haderka [ 13/Jul/15 ]

If nothing else, test images can't be on the root of the project, they belong in test resources so they are not bundled with the release.

Comment by Philip Mundt [ 13/Jul/15 ]

I second had comment, I guess those images were accidentally added:

magnolia-module-imaging/CropAndResizeQualityTest-quality1-expected.jpg
magnolia-module-imaging/CropAndResizeQualityTest-quality1-expected.png
magnolia-module-imaging/CropAndResizeQualityTest-quality1-original.jpg
magnolia-module-imaging/CropAndResizeQualityTest-quality1-original.png
magnolia-module-imaging/CropAndResizeQualityTesttest-quality1-result-basic.jpg
magnolia-module-imaging/CropAndResizeQualityTesttest-quality1-result-basic.png
magnolia-module-imaging/CropAndResizeQualityTesttest-quality1-result-multistep.jpg
magnolia-module-imaging/CropAndResizeQualityTesttest-quality1-result-multistep.png
magnolia-module-imaging/CropAndResizeQualityTesttest-quality1-result-scalearea-averaging.jpg
magnolia-module-imaging/CropAndResizeQualityTesttest-quality1-result-scalearea-averaging.png
magnolia-module-imaging/MultiStepResizerTesttest-result-multistep.jpg
magnolia-module-imaging/gif2gif.gif
magnolia-module-imaging/gif2jpg.jpg
magnolia-module-imaging/gif2png.png
magnolia-module-imaging/jpg2gif.gif
magnolia-module-imaging/jpg2jpg.jpg
magnolia-module-imaging/jpg2png.png
magnolia-module-imaging/png2gif.gif
magnolia-module-imaging/png2jpg.jpg
magnolia-module-imaging/png2png.png
magnolia-module-imaging/pngWithAlpha2Png.png
magnolia-module-imaging/pngWithAlpha2PngBlackBackground.png
magnolia-module-imaging/test-blank.jpg
magnolia-module-imaging/test-blank.png
magnolia-module-imaging/test-result.png
Comment by Jan Haderka [ 13/Jul/15 ]

Another thing that I've already mentioned earlier - default for this should be same as it was in the past - "true" and DAM should only enable the change (by setting property explicitly to false via version handler) when it's actually fixed and strips off extensions consistently.

+    private boolean trimExtension = false;
...
-        final PathSplitter pathSplitter = new PathSplitter(pathInfo, true);
+        final PathSplitter pathSplitter = new PathSplitter(pathInfo, trimExtension);
Comment by Magnolia International [ 13/Jul/15 ]

Can somebody explain why this is a DAM regression and why it needs to be fixed in Imaging ?

Comment by Antonín Juran [ 13/Jul/15 ]

For 3.1.4 version is fixed on MGNLIMG-163 branch, for 3.2.x version is fixed on MGNLIMG-163-3.2.x branch.

Comment by Antonín Juran [ 13/Jul/15 ]

Add test for case when trimExtension = false.

Comment by Jan Haderka [ 14/Jul/15 ]

To (partially) answer your question: Change happening here is making behaviour of appending image type extension to the link configurable. This was not necessary in the past since DAM was always stripping of extension from the nodes. And imaging (to follow the suite) was stripping extension from the links for which it was searching image. The recent version of DAM (one released with 5.3.8) changed it's default behaviour and started to upload images w/ extensions, thus it is no longer necessary for imaging to strip extension when searching for node.
This led to the need of users to add extension twice to make sure that when stripped, reminder of the link is still matching node in DAM with the extension in the name.
Because there might be some custom code using imaging or there might be some other chains loading images from other workspaces than DAM, it is not simply possible to change behaviour here and expect that it would not break anything else. Solution therefore makes behaviour configurable to allow users of functionality to change whether extension is stripped when searching for node or not per chain of image operations.

Comment by Magnolia International [ 14/Jul/15 ]

In ImagingServlet, we added response.setContentType("image/" + outputFormat);
Two things:

  1. There is TODO just above that that WAS saying we should take care of "mimetype etc". It looks like we do now, so why not get rid of the TODO?
  2. This is usually taken care of by the ContentTypeFilter (based on extension, which is perhaps a problem here). If that doesn't work correctly and we need to do it explicitly in this servlet, then we need to document why (i.e with a simple inline comment)
Comment by Antonín Juran [ 14/Jul/15 ]

1. Answer to the question
The original ticket was created for DAM issue but when I was solving it I discovered, that the issue isn't in DAM but in Imaging module. So I moved the ticket to Imaging.
2. Description of the fix
in AbstractWorkspaceAndPathParameterProviderFactory, AbstractWorkspaceAndUuidParameterProviderFactory classes in ver. 3.1.4 and into AbstractWorkspaceAndIdentifierParameterProviderFactory, AbstractWorkspaceAndPathParameterProviderFactory, AbstractWorkspaceAndUuidParameterProviderFactory in ver. 3.2.1 was added private boolean field trimExtension and its setter method, which allows setting of trimming or not trimming of extension in configuration. Default value is true – trimming of extension to be backward compatible, but once linked DAM issue - MGNLDAM-589 is fixed and extensions are consistently made part of the asset name, DAM (or STK or MTE) can via simple udate task change the default value to switch of trimming for whichever generator will be used to serve images.
Alternatively we can change default value here, but with a risk of affecting other custom generators that serve images from other locations than DAM.

Comment by Antonín Juran [ 14/Jul/15 ]

In ImagingServlet was added inline comment response.setContentType("image/" + outputFormat) and removed TODO comment.

Generated at Mon Feb 12 02:12:54 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.