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

Refactor Url and Href #388

Merged
merged 24 commits into from
Sep 19, 2023
Merged

Conversation

mickael-menu
Copy link
Member

Various refactoring to improve the safety and reliability of URLs and Link HREFs.

Rationale

  • The legacy handling of HREFs as percent-decoded path was source of many issues (the most recent) with a fragile normalization strategy.
  • The use of strings when a URL is expected didn't take advantage of static checks to verify that we pass valid URLs, or check that we require an absolute URLs.
  • Link templated HREFs were too easy to ignore.

Changes

  • A new Url sealed class with two implementations AbsoluteUrl and RelativeUrl.
    • AbsoluteUrl offers helpers to check the URL scheme and convert to/from a File.
    • There are helpers to resolve a relative URL to a base URL, or to make an absolute URL relative. These replace the old Href() normalization strategy.
    • To be valid, a Url is always percent-encoded. We can encode a relative path (e.g. a ZIP entry name) with Url.fromDecodedPath().
  • The old util.Href doesn't exist anymore. There's a publication.Href which can be either a URI template, or a valid Url. It is used in Link objects.
  • Link and Locator expose Url and Href instead of raw strings.
  • To prevent breaking implementations, the OPDS 2 parser is still normalizing all the HREFs to self or the feed URL. I added some kind of Visitor to modify the Manifest to normalize the HREFs (ManifestTransformer).

@CodiumAI-Agent
Copy link

PR Analysis

  • 🎯 Main theme: Refactoring of URL and HREF handling in the Readium project
  • 📝 PR summary: This PR introduces significant changes to the handling of URLs and HREFs in the Readium project. It introduces a new Url sealed class with two implementations AbsoluteUrl and RelativeUrl, and replaces the old util.Href with a new publication.Href which can be either a URI template, or a valid Url. The changes aim to improve safety and reliability, and address issues with the legacy handling of HREFs.
  • 📌 Type of PR: Refactoring
  • 🧪 Relevant tests added: Yes
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR is well-structured and the changes are well-documented, making it easy to understand the rationale behind the changes. The use of a sealed class for Url is a good design choice, as it provides a clear distinction between absolute and relative URLs. The refactoring of Href also seems to be a positive change, as it provides a clear distinction between a URI template and a valid Url. The changes appear to be comprehensive, touching all parts of the codebase where URLs and HREFs are used.

  • 🤖 Code feedback:

    • relevant file: readium/shared/src/test/java/org/readium/r2/shared/publication/PublicationTest.kt
      suggestion: Consider adding more edge case tests for the new Url and Href classes. For example, tests for handling of special characters in URLs, or tests for relative URLs without a base URL. [medium]
      relevant line: import org.readium.r2.shared.urlHref

    • relevant file: readium/shared/src/main/java/org/readium/r2/shared/publication/Link.kt
      suggestion: It might be beneficial to add some error handling or logging in case of invalid URLs or HREFs. This could help with debugging and identifying issues in the future. [medium]
      relevant line: Link(href = urlHref("http://domain.com/path/manifest.json"), rels = setOf("self"))

How to use

Tag me in a comment '@CodiumAI-Agent' and add one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

qnga and others added 9 commits September 15, 2023 16:10
# Conflicts:
#	readium/lcp/src/main/java/org/readium/r2/lcp/license/container/ContainerLicenseContainer.kt
#	readium/lcp/src/main/java/org/readium/r2/lcp/license/container/FileZipLicenseContainer.kt
#	readium/lcp/src/main/java/org/readium/r2/lcp/license/container/LcplLicenseContainer.kt
#	readium/lcp/src/main/java/org/readium/r2/lcp/license/container/LcplResourceLicenseContainer.kt
#	readium/lcp/src/main/java/org/readium/r2/lcp/license/container/LicenseContainer.kt
#	readium/shared/src/main/java/org/readium/r2/shared/publication/services/content/iterators/HtmlResourceContentIterator.kt
#	test-app/src/main/java/org/readium/r2/testapp/bookshelf/BookRepository.kt
#	test-app/src/main/java/org/readium/r2/testapp/catalogs/CatalogViewModel.kt
@mickael-menu mickael-menu merged commit 9fe0d30 into fix/remove-root-prefix Sep 19, 2023
3 checks passed
@mickael-menu mickael-menu deleted the refactor-href-url branch September 19, 2023 14:59
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.

3 participants