Skip to content
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

Store images on IPFS #1936

Merged
merged 2 commits into from
Nov 8, 2024
Merged

Store images on IPFS #1936

merged 2 commits into from
Nov 8, 2024

Conversation

jo-elimu
Copy link
Member

@jo-elimu jo-elimu commented Nov 8, 2024

Issue Number

Purpose

  • Move file storage out of the database, to make the webapp more lightweight.

Technical Details

  • API key and API secret are needed for testing on localhost.

Testing Instructions

  • Go to the image list, and press an image. That will trigger a request to Pinata.

Screenshots

The red/orange color indicates that an image has not yet been pinned to IPFS (does not have a CID):


Format Checks

Note

Files in PRs are automatically checked for format violations with mvn spotless:check.

If this PR contains files with format violations, run mvn spotless:apply to fix them.

@jo-elimu jo-elimu self-assigned this Nov 8, 2024
@jo-elimu jo-elimu requested a review from a team as a code owner November 8, 2024 13:23
Copy link

codecov bot commented Nov 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 49 lines in your changes missing coverage. Please review.

Project coverage is 16.00%. Comparing base (c9c1114) to head (bebc4e7).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/main/java/ai/elimu/util/IpfsHelper.java 0.00% 16 Missing ⚠️
.../content/multimedia/image/ImageEditController.java 0.00% 9 Missing ⚠️
...t/storybook/StoryBookCreateFromEPubController.java 0.00% 9 Missing ⚠️
.../java/ai/elimu/model/content/multimedia/Image.java 0.00% 6 Missing ⚠️
...ontent/multimedia/image/ImageCreateController.java 0.00% 4 Missing ⚠️
.../web/context/EnvironmentContextLoaderListener.java 0.00% 4 Missing ⚠️
...main/java/ai/elimu/rest/v2/JpaToGsonConverter.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1936      +/-   ##
============================================
+ Coverage     15.93%   16.00%   +0.06%     
  Complexity      482      482              
============================================
  Files           257      257              
  Lines          7871     7837      -34     
  Branches        821      807      -14     
============================================
  Hits           1254     1254              
+ Misses         6567     6533      -34     
  Partials         50       50              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

coderabbitai bot commented Nov 8, 2024

Walkthrough

The pull request introduces significant changes to the image handling and storage mechanisms within the application. The Image class has been updated to include a new cid field for storing IPFS Content Identifiers, while the deprecated bytes field is still present. The handling of image URLs has been streamlined with the addition of a getUrl() method. The ImageHelper class has been removed, and its functionality has been replaced with the new IpfsHelper, which facilitates interactions with the IPFS service. Various controllers and JSP files have been modified to accommodate these changes, focusing on IPFS integration and simplifying image processing logic.

Changes

File Path Change Summary
src/main/java/ai/elimu/model/content/multimedia/Image.java - Added field cid for IPFS identifier.
- Added method getUrl() for URL generation.
- Deprecated bytes field.
- Added getter/setter for cid.
src/main/java/ai/elimu/rest/v2/JpaToGsonConverter.java - Updated getImageGson method to use image.getUrl() for bytesUrl.
src/main/java/ai/elimu/util/ImageHelper.java - Removed class ImageHelper and its methods.
src/main/java/ai/elimu/util/IpfsHelper.java - Added new class IpfsHelper with method pinFileToIpfs for IPFS interactions.
src/main/java/ai/elimu/web/content/multimedia/image/ImageCreateController.java - Removed WordDao dependency.
- Added functionality to pin images to IPFS after creation.
src/main/java/ai/elimu/web/content/multimedia/image/ImageEditController.java - Replaced ImageHelper with IpfsHelper.
- Added IPFS pinning logic in handleRequest and handleSubmit.
src/main/java/ai/elimu/web/content/multimedia/video/VideoCreateController.java - Removed image width validation logic for thumbnails.
src/main/java/ai/elimu/web/content/storybook/StoryBookCreateFromEPubController.java - Updated to use IpfsHelper for image handling.
src/main/java/ai/elimu/web/context/EnvironmentContextLoaderListener.java - Added properties for Pinata API key and secret.
src/main/resources/META-INF/jpa-schema-export.sql - Dropped multiple tables and added cid column to the Image table.
src/main/resources/config.properties - Added properties for pinata.api.key and pinata.api.secret.
src/main/webapp/WEB-INF/jsp/content/multimedia/image/edit.jsp - Updated image source URL to use ${image.url}.
src/main/webapp/WEB-INF/jsp/content/multimedia/image/list.jsp - Updated card image display to use image.url.
src/main/webapp/WEB-INF/jsp/content/storybook/edit.jsp - Updated image source for storybook chapters to use storyBookChapter.image.url.
src/main/webapp/WEB-INF/jsp/content/storybook/paragraph/create.jsp - Updated image source to use storyBookChapter.image.url.
src/main/webapp/WEB-INF/jsp/content/storybook/paragraph/edit.jsp - Updated image source to use storyBookChapter.image.url.
src/main/webapp/WEB-INF/jsp/content/word/edit.jsp - Updated image source in labeledImages to use image.url.
src/main/webapp/static/css/styles.css - Added CSS rule for .card.image .card-image.cid-false.

Assessment against linked issues

Objective Addressed Explanation
Store image files on IPFS instead of in the database (#1933)
Add CID to REST API (#1933)
Handle scenario where Pinata API key/secret is missing (#1933) Implementation details unclear.

Possibly related PRs

  • refactor: move /contributor #1897: The changes in the Image class regarding the introduction of the cid field and the getUrl() method are related to the modifications in the JpaToGsonConverter class, where the bytesUrl property is now set using the getUrl() method of the Image object.

Suggested reviewers

  • vrudas
  • alexander-kuruvilla
  • Souvik-Cyclic
  • nya-elimu

Warning

Tool Failures:

Tool Failure Count:


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 20

🧹 Outside diff range and nitpick comments (16)
src/main/resources/config.properties (1)

14-15: Enhance documentation for IPFS setup

While the basic setup instructions are provided, consider adding:

  1. Link to IPFS documentation
  2. Steps to verify successful configuration
  3. Troubleshooting guidelines
  4. Production deployment considerations

Suggested documentation enhancement:

# To upload multimedia files during localhost development (and have them pinned to IPFS), 
# you need a Pinata API key & API secret. You can obtain those at https://app.pinata.cloud/
+# 
+# Setup Instructions:
+# 1. Sign up at https://app.pinata.cloud/
+# 2. Generate API credentials from your dashboard
+# 3. Set credentials in environment variables:
+#    export PINATA_API_KEY=your_key
+#    export PINATA_API_SECRET=your_secret
+# 4. Verify setup by uploading a test image
+#
+# For production deployment, refer to deployment guide at docs/deployment.md
src/main/webapp/WEB-INF/jsp/content/storybook/paragraph/create.jsp (1)

10-10: Consider adding null check for URL.

While the parent conditional checks for image existence, it's good practice to also check if the URL itself is not null to prevent potential rendering issues.

-                <img src="<spring:url value='${storyBookParagraph.storyBookChapter.image.url}' />" alt="${storyBookParagraph.storyBookChapter.storyBook.title}" />
+                <c:if test="${not empty storyBookParagraph.storyBookChapter.image.url}">
+                    <img src="<spring:url value='${storyBookParagraph.storyBookChapter.image.url}' />" alt="${storyBookParagraph.storyBookChapter.storyBook.title}" />
+                </c:if>
src/main/java/ai/elimu/util/IpfsHelper.java (1)

18-24: Enhance method documentation.

The Javadoc should include information about exceptions and null handling.

Update the documentation:

 /**
  * PIN file to IPFS.
  * 
  * @param bytes The file bytes.
  * @param filename The file name.
  * @return The Content Identifier (CID) generated by Pinata.
+ * @throws IllegalArgumentException if bytes is null/empty or filename is null/empty
+ * @throws IllegalStateException if Pinata API credentials are not configured
+ * @throws RuntimeException if pinning to IPFS fails
  */
src/main/java/ai/elimu/model/content/multimedia/Image.java (2)

17-22: Document deprecation timeline and migration strategy.

While marking the bytes field as deprecated is correct for the IPFS transition, we should:

  1. Add @Deprecated(since = "version", forRemoval = true) with version info
  2. Document the migration timeline in the deprecation notice
  3. Consider making @NotNull optional during transition
-    @Deprecated
+    @Deprecated(since = "2.0.0", forRemoval = true)
+    /**
+     * @deprecated This field will be removed in version X.X.X. 
+     * Images are being migrated to IPFS storage. Use {@link #cid} instead.
+     */
     @NotNull
     @Lob
     @Column(length=10485760) // 10MB
     private byte[] bytes;

60-67: Add validation and documentation to CID accessors.

The CID getter/setter methods should include:

  1. Input validation in setCid
  2. Javadoc documentation
+    /**
+     * @return The IPFS Content Identifier of the image, or null if not yet stored on IPFS
+     */
     public String getCid() {
         return cid;
     }

+    /**
+     * @param cid The IPFS Content Identifier (must be a valid CIDv0 or CIDv1)
+     * @throws IllegalArgumentException if the CID format is invalid
+     */
     public void setCid(String cid) {
+        if (cid != null && !cid.matches("^(Qm[1-9A-HJ-NP-Za-km-z]{44}|bafy[0-9A-Za-z]+)$")) {
+            throw new IllegalArgumentException("Invalid CID format: " + cid);
+        }
         this.cid = cid;
     }
src/main/webapp/WEB-INF/jsp/content/storybook/paragraph/edit.jsp (1)

11-11: Consider adding fallback handling for IPFS URLs.

If the IPFS service is unavailable or the image hasn't been pinned yet, the current implementation might show a broken image.

Consider adding an onerror handler:

-                <img src="<spring:url value='${storyBookParagraph.storyBookChapter.image.url}' />" alt="${storyBookParagraph.storyBookChapter.storyBook.title}" />
+                <img 
+                    src="<spring:url value='${storyBookParagraph.storyBookChapter.image.url}' />" 
+                    alt="${storyBookParagraph.storyBookChapter.storyBook.title}"
+                    onerror="this.src='<spring:url value='/static/images/image-placeholder.png' />'; this.classList.add('load-error');"
+                />
src/main/webapp/static/css/styles.css (1)

118-120: LGTM! Consider adding a hover tooltip for clarity.

The orangered border effectively indicates images not yet pinned to IPFS. The implementation is clean and consistent with existing styles.

Consider adding a tooltip to explain the border's meaning to users:

 .card.image .card-image.cid-false {
     border-top: 8px solid orangered;
+    position: relative;
 }
+.card.image .card-image.cid-false::before {
+    content: "Not yet stored on IPFS";
+    position: absolute;
+    top: -8px;
+    right: 0;
+    background: orangered;
+    color: white;
+    padding: 4px 8px;
+    border-radius: 4px;
+    font-size: 12px;
+    opacity: 0;
+    transition: opacity 0.2s;
+}
+.card.image .card-image.cid-false:hover::before {
+    opacity: 1;
+}
src/main/java/ai/elimu/web/context/EnvironmentContextLoaderListener.java (1)

126-131: Consider adding documentation for Pinata API configuration.

Since this is a new integration point, consider adding:

  1. Comments explaining the purpose of these properties
  2. Documentation in README/wiki about configuring Pinata credentials
src/main/java/ai/elimu/rest/v2/JpaToGsonConverter.java (1)

209-209: Consider standardizing URL generation across multimedia types

The getUrl() method approach used here for images could be extended to other multimedia types (Audio, Video) for consistency. This would make future storage migrations easier to implement.

Consider creating a common interface or base class for multimedia URL generation that can be implemented by each media type. This would make it easier to:

  1. Switch storage backends
  2. Maintain consistent URL patterns
  3. Handle CDN integration
  4. Implement caching strategies
src/main/webapp/WEB-INF/jsp/content/multimedia/image/edit.jsp (3)

Line range hint 89-95: Update file upload field name to reflect IPFS storage

The form is still using the deprecated bytes field for file upload. Consider updating the field name to better reflect the new IPFS-based storage approach.

 <div class="file-field input-field col s12">
     <div class="btn">
         <span><fmt:message key='file' /></span>
-        <form:input path="bytes" type="file" />
+        <form:input path="imageFile" type="file" />
     </div>
     <div class="file-path-wrapper">
         <input class="file-path validate" type="text" />
     </div>
 </div>

Line range hint 8-14: Display IPFS Content Identifier (CID)

To improve transparency and align with the PR objective of exposing CIDs, consider adding a display field for the IPFS Content Identifier.

 <form:form modelAttribute="image" enctype="multipart/form-data">
     <tag:formErrors modelAttribute="image" />
     
     <form:hidden path="revisionNumber" value="${image.revisionNumber}" />
     <form:hidden path="imageFormat" value="${image.imageFormat}" />
     <form:hidden path="contentType" value="${image.contentType}" />
     <form:hidden path="dominantColor" value="${image.dominantColor}" />
+    <c:if test="${not empty image.cid}">
+        <div class="input-field col s12">
+            <i class="material-icons prefix">fingerprint</i>
+            <input type="text" value="${image.cid}" readonly="readonly" />
+            <label>IPFS Content Identifier (CID)</label>
+        </div>
+    </c:if>
     <input type="hidden" name="timeStart" value="${timeStart}" />

Line range hint 1-24: Add error handling for IPFS operations

The PR objectives mention handling scenarios where Pinata API credentials are missing. Consider adding error messages to inform users when IPFS operations fail.

Add the following error handling:

 <content:section cssId="imageEditPage">
     <h4><content:gettitle /></h4>
+    <c:if test="${not empty ipfsError}">
+        <div class="card-panel red lighten-3">
+            <b>Error:</b> Unable to perform IPFS operation. ${ipfsError}
+        </div>
+    </c:if>
     
     <div class="card-panel">
src/main/resources/META-INF/jpa-schema-export.sql (2)

300-300: Consider adding constraints to the cid column.

The cid column has been added to support IPFS integration, but it lacks constraints that could help maintain data integrity during and after migration.

Consider adding these constraints after the migration is complete:

-        cid varchar(255),
+        cid varchar(255) UNIQUE,

Line range hint 292-305: Consider adding an index on the cid column.

Since the cid column will be used for lookups to retrieve images from IPFS, adding an index would improve query performance.

Add an index after the table creation:

CREATE INDEX idx_image_cid ON Image(cid);
src/main/java/ai/elimu/web/content/multimedia/image/ImageEditController.java (1)

90-96: Refactor duplicated IPFS pinning logic

The code for pinning the image to IPFS and updating the cid is duplicated in both the handleRequest (lines 90-96) and handleSubmit (lines 195-199) methods. To improve maintainability and reduce code duplication, consider extracting this logic into a separate private method.

Apply this refactor to extract the duplicated logic:

private void pinImageToIpfs(HttpServletRequest request, Image image) {
    String sanitizedTitle = image.getTitle().replaceAll("[^a-zA-Z0-9\\.\\-]", "_");
    String filename = request.getServerName() + "_image" + image.getId() + "-r" + image.getRevisionNumber() + "_" + sanitizedTitle;
    try {
        String ipfsHash = IpfsHelper.pinFileToIpfs(image.getBytes(), filename);
        image.setCid(ipfsHash);
        imageDao.update(image);
    } catch (Exception e) {
        logger.error("Failed to pin image to IPFS", e);
        // Handle the error appropriately
    }
}

Then update the methods accordingly:

 // In handleRequest method
 if (image.getCid() == null) {
-    // Pin file to IPFS
-    String filename = request.getServerName() + "_image" + image.getId() + "-r" + image.getRevisionNumber() + "_" + image.getTitle();
-    String ipfsHash = IpfsHelper.pinFileToIpfs(image.getBytes(), filename);
-    image.setCid(ipfsHash);
-    imageDao.update(image);
+    pinImageToIpfs(request, image);
 }

 // In handleSubmit method
- // Pin file to IPFS
- String filename = request.getServerName() + "_image" + image.getId() + "-r" + image.getRevisionNumber() + "_" + image.getTitle();
- String ipfsHash = IpfsHelper.pinFileToIpfs(image.getBytes(), filename);
- image.setCid(ipfsHash);
- imageDao.update(image);
+ pinImageToIpfs(request, image);

Also applies to: 195-199

src/main/java/ai/elimu/web/content/storybook/StoryBookCreateFromEPubController.java (1)

330-336: Refactor duplicated IPFS pinning code into a separate method

The code for pinning images to IPFS is duplicated in lines 330-336 and 385-390. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider extracting this logic into a separate method, such as pinImageToIpfs(Image image, HttpServletRequest request).

Also applies to: 385-390

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c9c1114 and bebc4e7.

⛔ Files ignored due to path filters (4)
  • pom.xml is excluded by !**/*.xml
  • src/main/config/centos-stream-9/opt/jetty-base/webapps/webapp-HIN.xml is excluded by !**/*.xml
  • src/main/config/centos-stream-9/opt/jetty-base/webapps/webapp-TGL.xml is excluded by !**/*.xml
  • src/main/config/centos-stream-9/opt/jetty-base/webapps/webapp.xml is excluded by !**/*.xml
📒 Files selected for processing (18)
  • src/main/java/ai/elimu/model/content/multimedia/Image.java (2 hunks)
  • src/main/java/ai/elimu/rest/v2/JpaToGsonConverter.java (1 hunks)
  • src/main/java/ai/elimu/util/ImageHelper.java (0 hunks)
  • src/main/java/ai/elimu/util/IpfsHelper.java (1 hunks)
  • src/main/java/ai/elimu/web/content/multimedia/image/ImageCreateController.java (3 hunks)
  • src/main/java/ai/elimu/web/content/multimedia/image/ImageEditController.java (3 hunks)
  • src/main/java/ai/elimu/web/content/multimedia/video/VideoCreateController.java (0 hunks)
  • src/main/java/ai/elimu/web/content/storybook/StoryBookCreateFromEPubController.java (3 hunks)
  • src/main/java/ai/elimu/web/context/EnvironmentContextLoaderListener.java (1 hunks)
  • src/main/resources/META-INF/jpa-schema-export.sql (1 hunks)
  • src/main/resources/config.properties (1 hunks)
  • src/main/webapp/WEB-INF/jsp/content/multimedia/image/edit.jsp (1 hunks)
  • src/main/webapp/WEB-INF/jsp/content/multimedia/image/list.jsp (1 hunks)
  • src/main/webapp/WEB-INF/jsp/content/storybook/edit.jsp (1 hunks)
  • src/main/webapp/WEB-INF/jsp/content/storybook/paragraph/create.jsp (1 hunks)
  • src/main/webapp/WEB-INF/jsp/content/storybook/paragraph/edit.jsp (1 hunks)
  • src/main/webapp/WEB-INF/jsp/content/word/edit.jsp (1 hunks)
  • src/main/webapp/static/css/styles.css (1 hunks)
💤 Files with no reviewable changes (2)
  • src/main/java/ai/elimu/util/ImageHelper.java
  • src/main/java/ai/elimu/web/content/multimedia/video/VideoCreateController.java
🔇 Additional comments (11)
src/main/webapp/WEB-INF/jsp/content/storybook/paragraph/create.jsp (1)

10-10: Verify URL escaping implementation.

The URL construction has been simplified appropriately to use the new IPFS-aware URL generation. However, let's verify that the URL is properly escaped to prevent XSS vulnerabilities.

✅ Verification successful

URL escaping is properly implemented across image URLs

The verification shows consistent usage of <spring:url value='...' /> for all image URL outputs across JSP files, including the modified file. This pattern ensures proper URL escaping and XSS protection.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if Spring's URL tag is consistently used for URL escaping across the codebase

# Test: Look for any direct usage of image.url without proper escaping
rg -g '*.jsp' "(?<!spring:url value='\$\{)image\.url"

# Test: Verify consistent usage of spring:url for image URLs
rg -g '*.jsp' "spring:url.*image\.url"

Length of output: 1425

src/main/java/ai/elimu/util/IpfsHelper.java (1)

1-13: LGTM! Well-organized imports.

The imports are specific, necessary, and properly organized.

src/main/java/ai/elimu/model/content/multimedia/Image.java (1)

17-28: Verify migration readiness and usage of deprecated field.

Let's ensure the codebase is ready for this transition.

src/main/webapp/WEB-INF/jsp/content/storybook/paragraph/edit.jsp (1)

11-11: Ensure XSS protection for the image URL.

The image URL is being output directly into HTML. While Spring's URL tag provides some protection, verify that the url property from Image class is properly escaped.

✅ Verification successful

XSS protection is properly implemented for image URLs

The implementation is secure for the following reasons:

  • The Image.getUrl() method returns either:
    • An IPFS URL with a content-addressed hash (CID) which is cryptographically secure
    • A local URL constructed from numeric ID and revision number
  • The URL is properly escaped by Spring's <spring:url> tag in the JSP, which handles URL encoding
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the Image class properly escapes the URL
ast-grep --pattern 'class Image {
  $$$
  getUrl() {
    $$$
  }
  $$$
}'

Length of output: 75


Script:

#!/bin/bash
# Let's try to find the Image class definition using ripgrep
rg -l "class Image" --type java

# Search for URL-related methods or fields in files containing "Image"
rg "url|URL" -A 5 -B 5 --type java -l "class Image"

# Look for Spring's URL escaping mechanisms in JSP files
rg -l "spring:url|escapeXml" --type jsp

Length of output: 1130


Script:

#!/bin/bash
# Let's examine the Image model class
cat src/main/java/ai/elimu/model/content/multimedia/Image.java

# Check JSP files with .jsp extension explicitly
rg -l "spring:url|escapeXml" --type-add 'jsp:*.jsp' -tjsp

Length of output: 4980

src/main/webapp/WEB-INF/jsp/content/multimedia/image/list.jsp (1)

29-29: Consider enhancing the UI feedback mechanism.

While the implementation correctly indicates IPFS status through CSS classes, consider these improvements:

  1. Use more semantic class names (e.g., ipfs-pinned instead of cid-true)
  2. Add a loading state during IPFS pinning
  3. Include fallback handling for failed image loads
-                        <div class="card-image cid-${image.cid != null}" style="background-image: url(<spring:url value='${image.url}' />);">
+                        <div class="card-image ipfs-${image.cid != null ? 'pinned' : 'unpinned'}"
+                             style="background-image: url(<spring:url value='${image.url}' />);"
+                             onerror="this.classList.add('image-load-error')">
+                            <div class="pinning-indicator" style="display: none;">
+                                <div class="progress">
+                                    <div class="indeterminate"></div>
+                                </div>
+                            </div>

Let's verify if there are corresponding CSS styles defined:

✅ Verification successful

The current implementation is sufficient, but could be enhanced

The codebase already has a working implementation for visual IPFS status indication using the cid-false class with an orange border. While your suggested improvements for semantic naming and error handling would be beneficial, they aren't critical since:

  1. The existing CSS class naming convention is consistently used and clearly indicates non-IPFS images
  2. The orange border provides clear visual feedback for non-IPFS images
  3. The background color (#ede7f6) serves as a natural placeholder/fallback for failed image loads

The current implementation meets the core requirements mentioned in the PR objectives. The suggested enhancements, while valuable, can be considered for future improvements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for CSS rules related to IPFS status indicators
rg -t css "\.cid-|\.ipfs-|\.image-load-error|\.pinning-indicator"

Length of output: 141


Script:

#!/bin/bash
# Let's check the full context of the CSS rules for card images
rg -A 5 -B 5 "card.image .card-image" src/main/webapp/static/css/styles.css

Length of output: 524

src/main/java/ai/elimu/web/context/EnvironmentContextLoaderListener.java (1)

127-131: Ensure secure handling of Pinata API credentials.

The implementation correctly follows the existing pattern for loading sensitive credentials. However, since these are sensitive API credentials:

  1. Ensure these credentials are properly secured in your deployment environment
  2. Consider adding validation to prevent null/empty values that could cause runtime issues

Let's verify the property loading implementation:

src/main/java/ai/elimu/web/content/multimedia/image/ImageCreateController.java (1)

4-4: LGTM! Import changes align with IPFS integration.

The removal of WordDao and addition of IpfsHelper imports correctly reflect the architectural shift from database storage to IPFS storage.

Also applies to: 22-22

src/main/webapp/WEB-INF/jsp/content/storybook/edit.jsp (1)

106-106: ⚠️ Potential issue

Ensure proper URL escaping and error handling.

While the change to use storyBookChapter.image.url aligns with the IPFS migration, there are a few concerns to address:

  1. The URL is rendered without proper escaping, which could lead to XSS vulnerabilities. Use <c:out> for the URL.
  2. There's no explicit null check for the URL property.

Apply this diff to fix the issues:

-                    <img src="<spring:url value='${storyBookChapter.image.url}' />" alt="${storyBook.title}" />
+                    <img src="<spring:url value='${fn:escapeXml(storyBookChapter.image.url)}' />" alt="<c:out value='${storyBook.title}' />" />

Let's verify the consistency of image URL handling across the codebase:

src/main/webapp/WEB-INF/jsp/content/word/edit.jsp (1)

447-447: LGTM! Verify URL generation for all image sources.

The change to use image.url aligns with the IPFS migration strategy. The URL generation is now handled by the Image model's getUrl() method, which provides a cleaner separation of concerns.

Let's verify that the URL generation works correctly for both IPFS and non-IPFS images:

✅ Verification successful

URL generation is correctly handled for both IPFS and non-IPFS images

Based on the code analysis, the Image class properly handles URL generation for both scenarios:

  • For IPFS images: Returns https://ipfs.io/ipfs/{cid}
  • For non-IPFS images: Returns /image/{id}_r{revisionNumber}.{format}

The change in the JSP is safe as it relies on this dual-mode URL generation implementation. The codebase also shows proper IPFS integration:

  • Images are automatically pinned to IPFS during creation/updates
  • The CID is stored in the database
  • The URL is dynamically generated based on the storage location
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Image class implementation handles both IPFS and non-IPFS URLs
# Expected: Find the Image class and verify getUrl() method implementation

# Search for the Image class definition and URL-related methods
ast-grep --pattern 'class Image {
  $$$
  getUrl() {
    $$$
  }
  $$$
}'

# Search for any test cases related to Image URL generation
rg -g '**/*Test.java' -A 5 'getUrl|image.*url'

Length of output: 124


Script:

#!/bin/bash
# Let's try different approaches to find the Image class and URL handling

# Search for Image class files
fd -e java -e kt Image

# Search for URL-related code in Java/Kotlin files
rg -t java -t kotlin "getUrl|setUrl|image.*url" -A 5

# Search for IPFS-related code
rg -t java -t kotlin "IPFS|ipfs" -A 5

# Look for Spring configuration related to image URLs
fd -e xml -e properties -e yml | xargs rg "image.*url|storage.*url"

Length of output: 19761

src/main/resources/META-INF/jpa-schema-export.sql (1)

299-301: Verify the migration strategy for coexisting storage methods.

The bytes column is retained while adding the new cid column, suggesting a transition period. This is a good approach for gradual migration, but ensure there's a clear strategy for:

  1. Handling new image uploads (whether to store in both locations)
  2. Migrating existing images to IPFS
  3. Cleaning up the bytes column after migration
✅ Verification successful

Based on the shell script results, I can now provide a final response:

Migration strategy for dual storage is properly implemented

The codebase shows a well-structured migration approach:

  1. The bytes column is retained while cid is added, allowing for gradual transition
  2. IPFS integration is implemented using Pinata SDK with proper configuration:
    • Pinata API credentials are configured in config.properties and webapp XML files
    • IpfsHelper class handles new uploads to IPFS and returns CIDs
  3. The system supports both storage methods simultaneously, enabling:
    • Storing new uploads in both locations during transition
    • Gradual migration of existing content
    • Eventual cleanup of legacy storage
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for existing images that need migration
# Look for non-null bytes columns that might need migration to IPFS

rg -l "INSERT INTO Image.*bytes.*[^NULL]" 

Length of output: 43


Script:

#!/bin/bash
# Let's check for any migration-related code or strategy
rg -i "migrat.*image" -A 5 -B 5

# Check for IPFS-related implementations
rg -i "ipfs.*upload|upload.*ipfs" -A 5 -B 5

# Look for image upload handling code
ast-grep --pattern 'class $_ {
  $$$
  upload($$$) {
    $$$
  }
  $$$
}'

# Check for any cleanup or migration scripts
fd -e sql -e java -e kt -e xml --exec rg -l "cleanup|migrat"

Length of output: 33672


Script:

#!/bin/bash
# Let's check for image-related service or repository implementations
ast-grep --pattern 'class $_ {
  $$$
  save($$$) {
    $$$
  }
  $$$
}'

# Look for image storage related configurations
rg -i "image.*storage|storage.*image" -A 5 -B 5

# Check for any IPFS service implementations
ast-grep --pattern 'class $_ {
  $$$
  pin($$$) {
    $$$
  }
  $$$
}'

# Look for Pinata-related code
rg -i "pinata" -A 5 -B 5

Length of output: 215224

src/main/java/ai/elimu/web/content/storybook/StoryBookCreateFromEPubController.java (1)

21-21: Import of IpfsHelper is appropriate

The IpfsHelper class is correctly imported for interacting with IPFS.

@nya-elimu nya-elimu merged commit 6fab712 into main Nov 8, 2024
12 of 14 checks passed
@nya-elimu nya-elimu deleted the 1933-images-on-ipfs branch November 8, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move storage of images from database to IPFS
2 participants