EDIT

A set of generic transformation rules for the BGBM digilib server rules will be stored in each db. This has been done for two reasons:

1. this allows for zero configuration usage of the digilib server
2. serves as a example or template on which other rules can be based on.
   

[
  {
    "height": 200,
    "width": 200,
    "mimeType": "image/jpeg",
    "pathQueryFragment": {
      "search": "digilib/Scaler/IIIF/([^\\!]+)\\!([^\\/]+)(.*)",
      "replace": "digilib/Scaler/IIIF/$1!$2/full/!200,200/0/default.jpg"
    },
    "host": {
      "search": "pictures.bgbm.org",
      "replace": "pictures.bgbm.org"
    },
    "scheme": null
  },
  {
    "height": 400,
    "width": 400,
    "mimeType": "image/jpeg",
    "pathQueryFragment": {
      "search": "digilib/Scaler/IIIF/([^\\!]+)\\!([^\\/]+)(.*)",
      "replace": "digilib/Scaler/IIIF/$1!$2/full/!400,400/0/default.jpg"
    },
    "host": {
      "search": "pictures.bgbm.org",
      "replace": "pictures.bgbm.org"
    },
    "scheme": null
  },
  {
    "height": 400,
    "width": 400,
    "mimeType": "image/jpeg",
    "pathQueryFragment": {
      "search": "digilib/Scaler/\\?fn=([^\\\\/]+)/(\\w+)(.*)",
      "replace": "digilib/Scaler/IIIF/$1!$2/full/!400,400/0/default.jpg"
    },
    "host": {
      "search": "pictures.bgbm.org",
      "replace": "pictures.bgbm.org"
    },
    "scheme": null
  },
  {
    "height": 200,
    "width": 200,
    "mimeType": "image/jpeg",
    "pathQueryFragment": {
      "search": "digilib/Scaler/\\?fn=([^\\\\/]+)/(\\w+)(.*)",
      "replace": "digilib/Scaler/IIIF/$1!$2/full/!200,200/0/default.jpg"
    },
    "host": {
      "search": "pictures.bgbm.org",
      "replace": "pictures.bgbm.org"
    },
    "scheme": null
  }
]

TODO: Skip the creation of a MediaRepresentation if a part with exactly the same URL already exists.

There is one issue that needs to be solved:

• MediaToolbox.readTransformations is creating default preference values and stores them in the database. This is not the correct way to handle preferences. Default values should be handled in the PreferencePredicate itself. This way the database is not littered by default value records. Also this way it is easier to change the default behavior without having to update database records. Simply by changing the code. So preferences should only be stored in the database if they deviate from the default value.

new default transformation for digilib:

[ {
  "scheme" : null,
  "host" : {
    "replace" : "pictures.bgbm.org",
    "search" : "pictures.bgbm.org"
  },
  "pathQueryFragment" : {
    "replace" : "digilib/Scaler/?fn=$1/$2&mo=crop&dw=200&dh=147&uvfix=1",
    "search" : "digilib/Scaler/IIIF/([^\\!]+)\\!([^\\/]+)(.*)"
  },
  "mimeType" : "image/jpeg",
  "width" : 200,
  "height" : 200
}, {
  "scheme" : null,
  "host" : {
    "replace" : "pictures.bgbm.org",
    "search" : "pictures.bgbm.org"
  },
  "pathQueryFragment" : {
    "replace" : "digilib/Scaler/IIIF/$1!$2/full/!400,400/0/default.jpg",
    "search" : "digilib/Scaler/IIIF/([^\\!]+)\\!([^\\/]+)(.*)"
  },
  "mimeType" : "image/jpeg",
  "width" : 400,
  "height" : 400
}, {
  "scheme" : null,
  "host" : {
    "replace" : "pictures.bgbm.org",
    "search" : "pictures.bgbm.org"
  },
  "pathQueryFragment" : {
    "replace" : "digilib/Scaler/IIIF/$1!$2/full/!400,400/0/default.jpg",
    "search" : "digilib/Scaler/\\?fn=([^\\\\/]+)/(\\w+)(.*)"
  },
  "mimeType" : "image/jpeg",
  "width" : 400,
  "height" : 400
}, {
  "scheme" : null,
  "host" : {
    "replace" : "pictures.bgbm.org",
    "search" : "pictures.bgbm.org"
  },
  "pathQueryFragment" : {
    "replace" : "digilib/Scaler/?fn=$1/$2&mo=crop&dw=200&dh=147&uvfix=1",
    "search" : "digilib/Scaler/\\?fn=([^\\\\/]+)/(\\w+)(.*)"
  },
  "mimeType" : "image/jpeg",
  "width" : 200,
  "height" : 200
} ]

Does it make sense to use

"width" : 200,
"height" : 200

if we use &mo=crop&dw=200&dh=147

Do we need width and heigth at all if we use crop?

Andreas Müller wrote:

...

• The MediaToolbox methods change the media itself by adding or removing representations. This might be dangerous in case the ToolBox is used whithin a not read-only transaction. Currently the toolbox belongs and is used only in remote to it is probably not used in such a way. However, to be on the save side it might be a good idea to use Media clones instead.

Using media clones can also cause problems: Rest service clients like the data portals may rely on the uuid of the media object as being a stable identifier. Cloned objects would have a different UUIDs than the original. Cloned objects with the same UUID - also not a good idea. May be we could set Media.id = 0, this would most probably cause an error when hibernate attempts to persist such an entity (not tested).

Another option is to return MediaDTOs instead of Media entities. It consider this as the best solution since it is more transparent than all others.

I implemented all of your suggestions and discovered a bug in my code:

media.getRepresentations().addAll(newRepr);

can you see the problem?

getRepresentations() must return an unmodifiableSet to avoid errors as above. I think we should protect all collections of entities where the add() and remove() methods do more than collection.add(), collection.addAll(), collection.remove(), etc.

Andreas Müller wrote:

Does it make sense to use

"width" : 200,
"height" : 200

if we use &mo=crop&dw=200&dh=147

Do we need width and heigth at all if we use crop?

I guess you mean the ./!400,400/0/default.jpg URLs? Theses are for the preview images in the taxon general page.

Andreas Kohlbecker wrote:

Andreas Müller wrote:

Does it make sense to use

"width" : 200,
"height" : 200

if we use &mo=crop&dw=200&dh=147

Do we need width and heigth at all if we use crop?

I guess you mean the ./!400,400/0/default.jpg URLs? Theses are for the preview images in the taxon general page.

Why 400? I know they are for the profile image. But above I mentioned

"width" : 200,
"height" : 200
if we use &mo=crop&dw=200&dh=147

so we talk about the thumbnails here. To me it sounds strange we have height=200 but dh=147 . But I did not go into detail. They might be a reason for this.

And the second issue was that I was wondering if we do need the width and heigth parameter at all for the algorithm if we use crop because crop will cut the image anyway to the size that is needed. But also here I did not really check against the code. It just came into my mind that this could be something that is not needed anymore with crop.

Andreas Kohlbecker wrote:

Andreas Müller wrote:

Using media clones can also cause problems: Rest service clients like the data portals may rely on the uuid of the media object as being a stable identifier. Cloned objects would have a different UUIDs than the original. Cloned objects with the same UUID - also not a good idea. May be we could set Media.id = 0, this would most probably cause an error when hibernate attempts to persist such an entity (not tested).

Hmm, true, using a clone does not fully solve the problem, as long as we do not make sure that we are out of an transaction. However, CdmBase.clone() sets the id of the new object to 0 anyway, so it is correct that setting the uuid to the old value will throw an exception if anyone will try to use this object within a transaction. So I think it is not dangerous to do it this way. However, a DTO might also be a good (and maybe more performant) solution but will take more time to implement.

Andreas Kohlbecker wrote:

Andreas Müller wrote:

I implemented all of your suggestions and discovered a bug in my code:

media.getRepresentations().addAll(newRepr);

can you see the problem?

getRepresentations() must return an unmodifiableSet to avoid errors as above. I think we should protect all collections of entities where the add() and remove() methods do more than collection.add(), collection.addAll(), collection.remove(), etc.

Yes, it is a known issue (discussed at the very early days) that in this context it is a bit dangerous to return the collections themself. I am not sure anymore why we decided to not do it the immutable way. One reason could be that in some cases you want to check if a collection is a PersistentCollection. You can't do this from outside if the collection that you get back is not the collection that is really attached to the object (except for using reflections which is a bit dirty and not performant). However, as this was never really an issue over the last 15 years (I remember only 1-2 cases where developers did this wrong) so I guess this is not really critical.

By the way, it can also be dangerous the other way round. E.g. we have the code

this.getRepresentations().remove(representation);

in Media.removeRepresentation(). This will not result in the expected behavior if getRepresentations() will not return the real set. Sometimes IDEs automatically do replace assignments by using getters and could create such problems.

Andreas Müller wrote:

• MediaUriTransformationProcessor.calculateTargetSize() does not handle maxextend transformations with trans.getWidth() != trans.getHeight() really correct by only returning "return new Point(trans.getWidth(), trans.getHeight());" . The algorithm should not depend on the fact that the required box is a square. I adapted the code accordingly (it even became simpler this way) and added tests. (193bfc9a54d2)

I actually don't expect that we will use maxextend transformations with trans.getWidth() != trans.getHeight() in the dataportal. I can't see the use case for this. More importantly for us is cropping of images. I modified the size calculation so that it works like:

• trans.getWidth() != trans.getHeight() ==> crop
• trans.getWidth() == trans.getHeight() ==> max extend

I adapted the test class also.

To cover all cases we would need to add a mode enum field to the MediaUriTransformation class with SCALE, MAX_EXTEND, CROP boolean field for isMaxextend

Andreas Müller wrote:

"width" : 200,
"height" : 200
if we use &mo=crop&dw=200&dh=147

so we talk about the thumbnails here. To me it sounds strange we have height=200 but dh=147 . But I did not go into detail. They might be a reason for this.

And the second issue was that I was wondering if we do need the width and heigth parameter at all for the algorithm if we use crop because crop will cut the image anyway to the size that is needed. But also here I did not really check against the code. It just came into my mind that this could be something that is not needed anymore with crop.

Now I got what you are referring to.

1. There actually was a bug in the code creating the default transformations - this is fixed, see bf5abe7ddd
2. The crop function crops the image to the specified size, that it to the width and height. The resulting image exactly has this width and size. There is no other way to define what is needed than defining the desired size.

the fixed default transformations as json for reference:

[ {
  "scheme" : null,
  "host" : {
    "replace" : "pictures.bgbm.org",
    "search" : "pictures.bgbm.org"
  },
  "pathQueryFragment" : {
    "replace" : "digilib/Scaler/?fn=$1/$2&mo=crop&dw=200&dh=147&uvfix=1",
    "search" : "digilib/Scaler/IIIF/([^\\!]+)\\!([^\\/]+)(.*)"
  },
  "mimeType" : "image/jpeg",
  "width" : 200,
  "height" : 147
}, {
  "scheme" : null,
  "host" : {
    "replace" : "pictures.bgbm.org",
    "search" : "pictures.bgbm.org"
  },
  "pathQueryFragment" : {
    "replace" : "digilib/Scaler/IIIF/$1!$2/full/!400,400/0/default.jpg",
    "search" : "digilib/Scaler/IIIF/([^\\!]+)\\!([^\\/]+)(.*)"
  },
  "mimeType" : "image/jpeg",
  "width" : 400,
  "height" : 400
}, {
  "scheme" : null,
  "host" : {
    "replace" : "pictures.bgbm.org",
    "search" : "pictures.bgbm.org"
  },
  "pathQueryFragment" : {
    "replace" : "digilib/Scaler/IIIF/$1!$2/full/!400,400/0/default.jpg",
    "search" : "digilib/Scaler/\\?fn=([^\\\\/]+)/(\\w+)(.*)"
  },
  "mimeType" : "image/jpeg",
  "width" : 400,
  "height" : 400
}, {
  "scheme" : null,
  "host" : {
    "replace" : "pictures.bgbm.org",
    "search" : "pictures.bgbm.org"
  },
  "pathQueryFragment" : {
    "replace" : "digilib/Scaler/?fn=$1/$2&mo=crop&dw=200&dh=147&uvfix=1",
    "search" : "digilib/Scaler/\\?fn=([^\\\\/]+)/(\\w+)(.*)"
  },
  "mimeType" : "image/jpeg",
  "width" : 200,
  "height" : 147
} ]

Andreas Kohlbecker wrote:

TODO: Skip the creation of a MediaRepresentation if a part with exactly the same URL already exists.

Is this fixed already? (I did not check the code)

The class SearchReplace is undocumented.
Also I wonder if we should maybe move it to cdmlib-common as it is context independent (neither dependent on service layer nor dependent on media) but simply defines a replace rule (have you checked if something like this maybe even exists somewhere in Java?).

Personally I would rename the class to ExpressionReplace(ment). To me the term "Search" is not so intuitive. Also the parameters "search" and "searchPattern" I would rename accordingly. But this is my personal opinion. We may keep the name if you do not agree.

Andreas Müller wrote:

• MediaToolbox is only logging JsonExceptions but not throwing them. Is this wanted (I am not familiar with the exception handling in remote therefore I ask)

This still seems to be the case. On purpose or simply forgotten?

Andreas Kohlbecker wrote:

I modified the code a bit so that there is only one method left which actually modifies media objects. All other methods only operate on MediaRepresentation list. In which case unwanted persisting of modified media objects is not a case to take into account. The last method which modifies Media is IMediaToolbox.filterPreferredMediaRepresentations(List<Media> mediaList, Class<? extends MediaRepresentationPart> type, String[] mimeTypes, Integer widthOrDuration, Integer height, Integer size). This method can be replaced easily and is no source of danger at the same time since is in only being used in the TaxonPortalController. I anyway set the entity id of the modified media entities to 0 to raise an error in case of an attempt to save it to the db.

To me it looks like processAndFilterPreferredMediaRepresentations() also still modifies media objects. There is the code:

for(MediaRepresentation r : newReprs) {
    media.addRepresentation(r);
}

which definetely modifies the media object. So we should either deprecate this method too or we keep both methods undeprecated and simply document that they may modify the input data and mention that there will be a DTO based method in future which does not do so. For me deprecating a method should only be done if it is either extremely dangerous to use (not really the case here, but maybe true) or if there is already an alternative to use.

did you also have a look to comments 26 - 30 ?

Beside comments 26-30 I would suggest to orally discuss if we really want to have the BGBM image server values as default or if these values should be stored in the according databases (cyprus, cichorieae). In general, the second solution feels more correct, but there might be reasons why to do it the first way. This can be done after the release.

Andreas Müller wrote:

Andreas Kohlbecker wrote:

I modified the code a bit so that there is only one method left which actually modifies media objects. All other methods only operate on MediaRepresentation list. In which case unwanted persisting of modified media objects is not a case to take into account. The last method which modifies Media is IMediaToolbox.filterPreferredMediaRepresentations(List<Media> mediaList, Class<? extends MediaRepresentationPart> type, String[] mimeTypes, Integer widthOrDuration, Integer height, Integer size). This method can be replaced easily and is no source of danger at the same time since is in only being used in the TaxonPortalController. I anyway set the entity id of the modified media entities to 0 to raise an error in case of an attempt to save it to the db.

To me it looks like processAndFilterPreferredMediaRepresentations() also still modifies media objects. There is the code:

fixed with 6348072d40

for(MediaRepresentation r : newReprs) {
    media.addRepresentation(r);
}

which definetely modifies the media object. So we should either deprecate this method too or we keep both methods undeprecated and simply document that they may modify the input data and mention that there will be a DTO based method in future which does not do so. For me deprecating a method should only be done if it is either extremely dangerous to use (not really the case here, but maybe true) or if there is already an alternative to use.

Andreas Müller wrote:

The class SearchReplace is undocumented.
Also I wonder if we should maybe move it to cdmlib-common as it is context independent (neither dependent on service layer nor dependent on media) but simply defines a replace rule (have you checked if something like this maybe even exists somewhere in Java?).

Personally I would rename the class to ExpressionReplace(ment). To me the term "Search" is not so intuitive. Also the parameters "search" and "searchPattern" I would rename accordingly. But this is my personal opinion. We may keep the name if you do not agree.

SearchReplace is specially dedicated to be used in MediaUriTransformation which is serializes to json cdm preference values.
I would prefer to keep it next to MediaUriTransformation. In principle is could be an public static inner class of the latter.

java doc added with 4c4eace63e

Andreas Müller wrote:

Andreas Müller wrote:

• MediaToolbox is only logging JsonExceptions but not throwing them. Is this wanted (I am not familiar with the exception handling in remote therefore I ask)

This still seems to be the case. On purpose or simply forgotten?

Nope, postponed, since this is a delicate one. A deserialization problem with the transformations as stored in the cdm preferences would cause all image viewers in the portal going blank. Throwing the exception in the readTransformations() and catching and swallowing it in controller methods would not be better than just logging the exception in the method where it occurs. I was thinking that we would need something like a health report for such errors that are really relevant for the maintainer but which should not break the user experience completely. This health report system could be attached to the logging framework where it would listen for ERROR level messages, collecting, assembling and counting severe events. Reports via UI or by daily email messages.

Andreas Kohlbecker wrote:

Andreas Müller wrote:

Andreas Müller wrote:

• MediaToolbox is only logging JsonExceptions but not throwing them. Is this wanted (I am not familiar with the exception handling in remote therefore I ask)

This still seems to be the case. On purpose or simply forgotten?

Nope, postponed, since this is a delicate one. A deserialization problem with the transformations as stored in the cdm preferences would cause all image viewers in the portal going blank. Throwing the exception in the readTransformations() and catching and swallowing it in controller methods would not be better than just logging the exception in the method where it occurs. I was thinking that we would need something like a health report for such errors that are really relevant for the maintainer but which should not break the user experience completely. This health report system could be attached to the logging framework where it would listen for ERROR level messages, collecting, assembling and counting severe events. Reports via UI or by daily email messages.

Should we create a new ticket for this and link to this note?

posting the SQL INSERT statement to add the default transformation to a DB:

INSERT INTO CdmPreference
(key_subject, key_predicate, value, allowOverride)
VALUES('/', 'media.representationTransformations', '[{"scheme":null,"host":{"replace":"pictures.bgbm.org","search":"pictures.bgbm.org"},"pathQueryFragment":{"replace":"digilib/Scaler/?fn=$1/$2&mo=crop&dw=200&dh=147&uvfix=1","search":"digilib/Scaler/IIIF/([^\\\\!]+)\\\\!([^\\\\/]+)(.*)"},"mimeType":"image/jpeg","width":200,"height":147,"maxExtend":false},{"scheme":null,"host":{"replace":"pictures.bgbm.org","search":"pictures.bgbm.org"},"pathQueryFragment":{"replace":"digilib/Scaler/IIIF/$1!$2/full/!400,400/0/default.jpg","search":"digilib/Scaler/IIIF/([^\\\\!]+)\\\\!([^\\\\/]+)(.*)"},"mimeType":"image/jpeg","width":400,"height":400,"maxExtend":true},{"scheme":null,"host":{"replace":"pictures.bgbm.org","search":"pictures.bgbm.org"},"pathQueryFragment":{"replace":"digilib/Scaler/IIIF/$1!$2/full/!400,400/0/default.jpg","search":"digilib/Scaler/\\\\?fn=([^\\\\\\\\/]+)/(\\\\w+)(.*)"},"mimeType":"image/jpeg","width":400,"height":400,"maxExtend":true},{"scheme":null,"host":{"replace":"pictures.bgbm.org","search":"pictures.bgbm.org"},"pathQueryFragment":{"replace":"digilib/Scaler/?fn=$1/$2&mo=crop&dw=200&dh=147&uvfix=1","search":"digilib/Scaler/\\\\?fn=([^\\\\\\\\/]+)/(\\\\w+)(.*)"},"mimeType":"image/jpeg","width":200,"height":147,"maxExtend":false}]', 1);

I improved the regex in the DefaultMediaTransformations for digilib, this is the new default:

[ {
  "scheme" : null,
  "host" : {
    "replace" : "pictures.bgbm.org",
    "search" : "pictures.bgbm.org"
  },
  "pathQueryFragment" : {
    "replace" : "digilib/Scaler/?fn=$1/$2&mo=crop&dw=200&dh=147&uvfix=1",
    "search" : "digilib/Scaler/IIIF/([^\\!]+)\\!([^\\/]+)(.*)"
  },
  "mimeType" : "image/jpeg",
  "width" : 200,
  "height" : 147,
  "maxExtend" : false
}, {
  "scheme" : null,
  "host" : {
    "replace" : "pictures.bgbm.org",
    "search" : "pictures.bgbm.org"
  },
  "pathQueryFragment" : {
    "replace" : "digilib/Scaler/IIIF/$1!$2/full/!400,400/0/default.jpg",
    "search" : "digilib/Scaler/IIIF/([^\\!]+)\\!([^\\/]+)(.*)"
  },
  "mimeType" : "image/jpeg",
  "width" : 400,
  "height" : 400,
  "maxExtend" : true
}, {
  "scheme" : null,
  "host" : {
    "replace" : "pictures.bgbm.org",
    "search" : "pictures.bgbm.org"
  },
  "pathQueryFragment" : {
    "replace" : "digilib/Scaler/IIIF/$1!$2/full/!400,400/0/default.jpg",
    "search" : "digilib/Scaler/?\\?fn=([^\\\\/]+)/(\\w+)(.*)"
  },
  "mimeType" : "image/jpeg",
  "width" : 400,
  "height" : 400,
  "maxExtend" : true
}, {
  "scheme" : null,
  "host" : {
    "replace" : "pictures.bgbm.org",
    "search" : "pictures.bgbm.org"
  },
  "pathQueryFragment" : {
    "replace" : "digilib/Scaler/?fn=$1/$2&mo=crop&dw=200&dh=147&uvfix=1",
    "search" : "digilib/Scaler/?\\?fn=([^\\\\/]+)/(\\w+)(.*)"
  },
  "mimeType" : "image/jpeg",
  "width" : 200,
  "height" : 147,
  "maxExtend" : false
} ]

as SQL INSERT stanement:

INSERT INTO CdmPreference
(key_subject, key_predicate, value, allowOverride)
VALUES('/', '[ { "scheme" : null, "host" : { "replace" : "pictures.bgbm.org", "search" : "pictures.bgbm.org" }, "pathQueryFragment" : { "replace" : "digilib/Scaler/?fn=$1/$2&mo=crop&dw=200&dh=147&uvfix=1", "search" : "digilib/Scaler/IIIF/([^\\!]+)\\!([^\\/]+)(.*)" }, "mimeType" : "image/jpeg", "width" : 200, "height" : 147, "maxExtend" : false }, { "scheme" : null, "host" : { "replace" : "pictures.bgbm.org", "search" : "pictures.bgbm.org" }, "pathQueryFragment" : { "replace" : "digilib/Scaler/IIIF/$1!$2/full/!400,400/0/default.jpg", "search" : "digilib/Scaler/IIIF/([^\\!]+)\\!([^\\/]+)(.*)" }, "mimeType" : "image/jpeg", "width" : 400, "height" : 400, "maxExtend" : true }, { "scheme" : null, "host" : { "replace" : "pictures.bgbm.org", "search" : "pictures.bgbm.org" }, "pathQueryFragment" : { "replace" : "digilib/Scaler/IIIF/$1!$2/full/!400,400/0/default.jpg", "search" : "digilib/Scaler/?\\?fn=([^\\\\/]+)/(\\w+)(.*)" }, "mimeType" : "image/jpeg", "width" : 400, "height" : 400, "maxExtend" : true }, { "scheme" : null, "host" : { "replace" : "pictures.bgbm.org", "search" : "pictures.bgbm.org" }, "pathQueryFragment" : { "replace" : "digilib/Scaler/?fn=$1/$2&mo=crop&dw=200&dh=147&uvfix=1", "search" : "digilib/Scaler/?\\?fn=([^\\\\/]+)/(\\w+)(.*)" }, "mimeType" : "image/jpeg", "width" : 200, "height" : 147, "maxExtend" : false } ]');