imageUrl calls content API one time too many #10006

enonic · Jan 31, 2023 · a56e9e9 · a56e9e9
1 parent 25772dc
commit a56e9e9
Show file tree

Hide file tree

Showing 10 changed files with 106 additions and 189 deletions.
diff --git a/modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/node/GetBinaryKeyCommand.java b/modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/node/GetBinaryKeyCommand.java
@@ -2,7 +2,6 @@
 
 import com.google.common.base.Preconditions;
 
-import com.enonic.xp.data.PropertyPath;
 import com.enonic.xp.node.AttachedBinary;
 import com.enonic.xp.node.Node;
 import com.enonic.xp.node.NodeId;
@@ -13,61 +12,26 @@ public class GetBinaryKeyCommand
 {
     private final BinaryReference binaryReference;
 
-    private final PropertyPath propertyPath;
-
     private final NodeId nodeId;
 
     private GetBinaryKeyCommand( final Builder builder )
     {
         super( builder );
         this.binaryReference = builder.binaryReference;
-        this.propertyPath = builder.propertyPath;
         this.nodeId = builder.nodeId;
     }
 
     public String execute()
     {
         final Node node = doGetById( this.nodeId );
-        if ( binaryReference != null )
-        {
-            return getByBinaryReference( node );
-        }
-        else
-        {
-            return getByPropertyPath( node );
-        }
-    }
-
-    private String getByBinaryReference( final Node node )
-    {
         final AttachedBinary attachedBinary = node.getAttachedBinaries().getByBinaryReference( this.binaryReference );
 
         if ( attachedBinary == null )
         {
             return null;
         }
 
-        return doGetBlobKey( attachedBinary );
-    }
-
-    private String getByPropertyPath( final Node node )
-    {
-        final BinaryReference binaryReference = node.data().getBinaryReference( this.propertyPath );
-
-        if ( binaryReference == null )
-        {
-            return null;
-        }
-
-        final AttachedBinary attachedBinary = node.getAttachedBinaries().getByBinaryReference( binaryReference );
-
-        return doGetBlobKey( attachedBinary );
-    }
-
-    private String doGetBlobKey( final AttachedBinary attachedBinary )
-    {
         return attachedBinary.getBlobKey();
-
     }
 
     public static Builder create()
@@ -80,8 +44,6 @@ public static class Builder
     {
         private BinaryReference binaryReference;
 
-        private PropertyPath propertyPath;
-
         private NodeId nodeId;
 
         public Builder binaryReference( final BinaryReference binaryReference )
@@ -90,12 +52,6 @@ public Builder binaryReference( final BinaryReference binaryReference )
             return this;
         }
 
-        public Builder propertyPath( final PropertyPath propertyPath )
-        {
-            this.propertyPath = propertyPath;
-            return this;
-        }
-
         public Builder nodeId( final NodeId nodeId )
         {
             this.nodeId = nodeId;
@@ -108,9 +64,7 @@ void validate()
             super.validate();
 
             Preconditions.checkNotNull( nodeId, "nodeId not set" );
-
-            Preconditions.checkArgument( propertyPath != null || binaryReference != null,
-                                         "Either propertyPath or binaryReference must be set" );
+            Preconditions.checkArgument( binaryReference != null, "binaryReference must be set" );
         }
 
         public GetBinaryKeyCommand build()

diff --git a/.../portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/AttachmentUrlBuilder.java b/.../portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/AttachmentUrlBuilder.java
@@ -5,7 +5,6 @@
 import com.enonic.xp.attachment.Attachment;
 import com.enonic.xp.attachment.Attachments;
 import com.enonic.xp.content.Content;
-import com.enonic.xp.content.ContentId;
 import com.enonic.xp.portal.url.AttachmentUrlParams;
 
 final class AttachmentUrlBuilder
@@ -38,14 +37,12 @@ protected void buildUrl( final StringBuilder url, final Multimap<String, String>
 
     private Content resolveContent()
     {
-        final ContentId contentId = new ContentIdResolver().
+        return new ContentResolver().
             portalRequest( this.portalRequest ).
             contentService( this.contentService ).
             id( this.params.getId() ).
             path( this.params.getPath() ).
             resolve();
-
-        return this.contentService.getById( contentId );
     }
 
     private Attachment resolveAttachment( final Content content )

diff --git a/...les/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/ContentIdResolver.java b/...les/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/ContentIdResolver.java
diff --git a/...s/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/ContentPathResolver.java b/...s/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/ContentPathResolver.java
@@ -1,6 +1,5 @@
 package com.enonic.xp.portal.impl.url;
 
-import com.enonic.xp.content.Content;
 import com.enonic.xp.content.ContentId;
 import com.enonic.xp.content.ContentPath;
 import com.enonic.xp.content.ContentService;
@@ -44,30 +43,14 @@ public ContentPath resolve()
     {
         if ( this.id != null )
         {
-            return resolvePath( this.id );
+            return this.contentService.getById( this.id ).getPath();
         }
 
-        if ( this.path != null )
+        if ( path == null )
         {
-            return resolvePath( this.path );
+            return this.portalRequest.getContentPath();
         }
 
-        return this.portalRequest.getContentPath();
-    }
-
-    private ContentPath resolvePath( final ContentId id )
-    {
-        final Content content = this.contentService.getById( id );
-        return content.getPath();
-    }
-
-    private ContentPath resolvePath( final ContentPath path )
-    {
-        if ( path.isAbsolute() )
-        {
-            return path;
-        }
-
-        return ContentPath.from( this.portalRequest.getContentPath(), path );
+        return this.path.isAbsolute() ? this.path : ContentPath.from( this.portalRequest.getContentPath(), this.path );
     }
 }
diff --git a/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/ContentResolver.java b/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/ContentResolver.java
@@ -0,0 +1,67 @@
+package com.enonic.xp.portal.impl.url;
+
+import com.enonic.xp.content.Content;
+import com.enonic.xp.content.ContentId;
+import com.enonic.xp.content.ContentPath;
+import com.enonic.xp.content.ContentService;
+import com.enonic.xp.portal.PortalRequest;
+
+final class ContentResolver
+{
+    private PortalRequest portalRequest;
+
+    private ContentId id;
+
+    private ContentPath path;
+
+    private ContentService contentService;
+
+    public ContentResolver portalRequest( final PortalRequest portalRequest )
+    {
+        this.portalRequest = portalRequest;
+        return this;
+    }
+
+    public ContentResolver id( final String value )
+    {
+        this.id = value != null ? ContentId.from( value ) : null;
+        return this;
+    }
+
+    public ContentResolver path( final String value )
+    {
+        this.path = value != null ? ContentPath.from( value ) : null;
+        return this;
+    }
+
+    public ContentResolver contentService( final ContentService value )
+    {
+        this.contentService = value;
+        return this;
+    }
+
+    public Content resolve()
+    {
+        if ( this.id != null )
+        {
+            return this.contentService.getById( this.id );
+        }
+
+        if ( path == null )
+        {
+            return this.portalRequest.getContent();
+        }
+
+        final ContentPath contentPath;
+        if ( path.isAbsolute() )
+        {
+            contentPath = path;
+        }
+        else
+        {
+            contentPath = ContentPath.from( this.portalRequest.getContentPath(), this.path );
+        }
+
+        return this.contentService.getByPath( contentPath );
+    }
+}
diff --git a/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/ImageUrlBuilder.java b/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/ImageUrlBuilder.java
@@ -1,13 +1,13 @@
 package com.enonic.xp.portal.impl.url;
 
 import java.nio.charset.StandardCharsets;
+import java.util.Objects;
 
 import com.google.common.collect.Multimap;
 import com.google.common.hash.Hashing;
 import com.google.common.io.Files;
 
 import com.enonic.xp.content.Content;
-import com.enonic.xp.content.ContentId;
 import com.enonic.xp.content.ContentNotFoundException;
 import com.enonic.xp.content.Media;
 import com.enonic.xp.portal.url.ImageUrlParams;
@@ -29,13 +29,12 @@ protected void buildUrl( final StringBuilder url, final Multimap<String, String>
     {
         super.buildUrl( url, params );
 
-        final ContentId id = resolveId();
-        final Media media = resolveMedia( id );
+        final Media media = resolveMedia();
         final String hash = resolveHash( media );
         final String name = resolveName( media );
         final String scale = resolveScale();
 
-        appendPart( url, id + ":" + hash );
+        appendPart( url, media.getId() + ":" + hash );
         appendPart( url, scale );
         appendPart( url, name );
 
@@ -52,22 +51,27 @@ private void addParamIfNeeded( final Multimap<String, String> params, final Stri
         }
     }
 
-    private Media resolveMedia( final ContentId id )
+    private Media resolveMedia()
     {
         final Content content;
-
         try
         {
-            content = this.contentService.getById( id );
+            content = new ContentResolver().portalRequest( this.portalRequest )
+                .contentService( this.contentService )
+                .id( this.params.getId() )
+                .path( this.params.getPath() )
+                .resolve();
         }
         catch ( ContentNotFoundException e )
         {
-            throw new WebException( HttpStatus.NOT_FOUND, String.format( "Image with [%s] id not found", id ), e );
+            throw new WebException( HttpStatus.NOT_FOUND, String.format( "Image [%s] not found",
+                                                                         Objects.requireNonNullElse( this.params.getId(),
+                                                                                                     this.params.getPath() ) ), e );
         }
-
         if ( !content.getType().isDescendantOfMedia() && !content.getType().isMedia() )
         {
-            throw WebException.notFound( String.format( "Image with [%s] id not found", id ) );
+            throw WebException.notFound( String.format( "Image with [%s] id not found", content.getId() ) );
+
         }
         return (Media) content;
     }
@@ -100,16 +104,6 @@ private String resolveName( final Media media )
         return name;
     }
 
-    private ContentId resolveId()
-    {
-        return new ContentIdResolver().
-            portalRequest( this.portalRequest ).
-            contentService( this.contentService ).
-            id( this.params.getId() ).
-            path( this.params.getPath() ).
-            resolve();
-    }
-
     private String resolveScale()
     {
         if ( this.params.getScale() == null )