Replace links in "fields" with linked to entites from "includes" #181

WillRochaThomas · 2024-05-06T14:35:39Z

Created this to address issue #8, which means it's currently not possible to access any linked entities except Assets through the client (e.g. a linked Entry).

Open to adapting this, based on feedback. I've not made it completely backwards compatible (the previous 'assets' field on the Entry struct is no longer populated), but I have bumped a major version to compensate for this. It could be made backwards compatible, it would just make the code a bit more convoluted in Contentful.Delivery.Entries.resolve_collection_response/1 and leave a slightly unclear design (as to why assets are treated differently to other links).

I'm sure the link resolution code itself could be improved too, I just need fresh eyes on it because I've been looking at it too long.

Summary of changes:

Resolve all link fields found within "items" in an API response, and replace the links with the actual entities if available from the "includes" section of API response. Inspired by https://github.com/contentful/contentful.js/blob/master/ADVANCED.md#link-resolution
Breaking change: 'assets' field on Entry struct is no longer populated. All Assets for an Entry are embedded directly within 'fields' now instead

WillRochaThomas · 2024-05-06T14:39:36Z

CHANGELOG.md

 ## 0.5.0

-* [#97](https://github.com/contentful-userland/contentful.ex/pull/97) switches underlying http adapter to `tesla` for higher flexibility. Thank you @OldhamMade
-* tests against Elixir 1.13 and updates required elixir version to 1.11
+- [#97](https://github.com/contentful-userland/contentful.ex/pull/97) switches underlying http adapter to `tesla` for higher flexibility. Thank you @OldhamMade


my markdown formatter removed these, I don't think it needs both a - and a * to format correctly

- Resolve all link fields found within "items" in an API response, and replace the links with the actual entities if available from the "includes" section of API response. Inspired by https://github.com/contentful/contentful.js/blob/master/ADVANCED.md#link-resolution - Breaking change: 'assets' field on Entry struct is no longer populated. All Assets for an Entry are embedded directly within 'fields' now instead

WillRochaThomas · 2024-05-10T15:00:40Z

lib/contentful/sys_data.ex

-  defstruct [:id, :revision, :version, :created_at, :updated_at, locale: nil, content_type: nil]
+  defstruct [
+    :id,
+    :revision,


version wasn't a field I saw documented in the list of common attributes for API entities, and that field was never used in the code, so have removed it

version is relatively important for some entities that we have. It's there to deal with possible race conditions - and it has been moved to a header a while back (even though some payloads can still include it).

I am fine dropping it here, since most reading operations don't care about the field either.

good to know, apologies I didn't find any documentation on it, but I must have missed it

WillRochaThomas · 2024-05-17T13:16:20Z

@asaaki @floriank I'd love some feedback on this 🙏. I've tested this out on our website, using our fork, and found 1 issue that I've since fixed within this PR (test case added to link_resolver_test.exs to ensure the code properly handles lists of Entry links). Everything else seemed fine.

floriank · 2024-05-17T13:31:39Z

Oh wow thanks!

Sorry for not being more attentive here, I didn't think that the elixir community was still interested ❤️

All the better and thanks for the effort. It's a holiday weekend in Berlin but I've set myself a reminder to come back to it next week 👌

floriank

Oh,

I am late - as usual!

First of all - thanks a ton! This is good work and I appreciate it a lot! I will run a test manually tonight and personally, I think this looks quite mergeable to me.

Calling in on @asaaki for a slow motion "NoOOOOooOO" from the sideline <3

floriank · 2024-05-22T07:46:27Z

CHANGELOG.md

@@ -2,83 +2,88 @@

 ## Unreleased

+## 1.0.0


I am not really comfortable with immediately going v1 here - although I recognize that this introduces a breaking change so it might be prudent to do so.

Sure, I'd take your direction on this, would you make this version 0.6.0 instead? I guess there's implicit instability in the version being less than 1, but I did recently have an experience with a library introducing a breaking change in a minor version, and it made what should have been a quick/simple update of our dependencies, turn into a protracted debugging task.

I mean, technically I already crossed the rubicon when I rewrote the lib between 0.1 and 0.2 - I think 0.6.0 would be fine actually, since I'd want more of the Delivery APIs covered for a v1.0.0.

floriank · 2024-05-22T07:49:39Z

lib/contentful/entry/link_resolver.ex

+  Inspired by https://github.com/contentful/contentful.js/blob/master/ADVANCED.md#link-resolution
+  """
+  @spec replace_in_situ(Entry.t(), map()) :: Entry.t()
+  def replace_in_situ(%Entry{fields: fields} = entry, %{} = includes) do


While I appreciate the Latin here - replace_locally or replace_in_position? Public APIs tend to be hard to rename 😉

Yeah, I didn't love this name, it probably is too obscure a phrase. I was struggling to think of a name that doesn't just repeat the name of the module (it started out as LinkResolver.resolve_links before I realised I was repeating myself). Out of the two options, replace_in_position feels clearer than replace_locally, I'm happy to rename to that. An alternative suggestion is replace_links_with_entities – just re-reading how I described the change in the pull request, this is the phrase I used...maybe that's a sign of that being the most accurate description for this function? What do you think?

I do like replace_links_with_entities a lot. It's longer, but it's clear and also bespoke as a function in the API <3

floriank · 2024-05-22T07:52:53Z

lib/contentful/sys_data.ex

-  defstruct [:id, :revision, :version, :created_at, :updated_at, locale: nil, content_type: nil]
+  defstruct [
+    :id,
+    :revision,


version is relatively important for some entities that we have. It's there to deal with possible race conditions - and it has been moved to a header a while back (even though some payloads can still include it).

I am fine dropping it here, since most reading operations don't care about the field either.

WillRochaThomas · 2024-05-22T13:13:52Z

I noticed one of the CI jobs failed, with this error

 Code Readability                                                              
┃ 
┃ [R] ↘ The alias `Contentful.Delivery.Entries` is not alphabetically ordered 
┃       among its group.
┃       lib/contentful/entry/link_resolver.ex:8:38 #(Contentful.Entry.LinkResolver)
┃ [R] ↘ The alias `Contentful.Entry` is not alphabetically ordered among its 
┃       group.
┃       test/contentful/entry/link_resolver_test.exs:6:21 #(Contentful.Entry.LinkResolverTest)

Please report incorrect results: https://github.com/rrrene/credo/issues

I'll fix that when I also rename the function as you have suggested.

WillRochaThomas · 2024-05-28T16:07:04Z

@floriank I think I've addressed all your feedback and fixed the ordering of aliases in some files so credo style checks now pass. I actually identified a simplification to the code in link_resolver too, removing some duplication. I pushed a second commit so it's easier for you to see what I've changed, but would suggest squashing these commits on merge & using the commit message from the first commit only. Let me know if there's anything I've missed!

coveralls · 2024-05-28T16:10:19Z

Pull Request Test Coverage Report for Build ad77c37f1c266f4dfa31049beaedc7eacddb3d4e-PR-181

Details

23 of 28 (82.14%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.0%) to 78.646%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/contentful/entry/link_resolver.ex	21	26	80.77%

Totals
Change from base Build c90493c4dccf45cf1a65f6bf2318c3e0cd00fc55:	-1.0%
Covered Lines:	151
Relevant Lines:	192

💛 - Coveralls

WillRochaThomas · 2024-06-20T15:16:13Z

@floriank @asaaki are we able to go ahead and merge this?

floriank · 2024-06-20T15:20:33Z

Yep!

I am just on summer vacation right now (without a laptop) but will return on the weekend!

(Apologies, bad timing is sort of my specialty 😬)

WillRochaThomas · 2024-06-24T09:34:43Z

Enjoy your vacation 😎!

WillRochaThomas commented May 6, 2024

View reviewed changes

This was referenced May 6, 2024

Links of links don't resolve properly #12

Open

Entries not loading relations #8

Open

WillRochaThomas force-pushed the resolve-linked-entries branch 3 times, most recently from a24a142 to 1c5109c Compare May 7, 2024 10:39

WillRochaThomas force-pushed the resolve-linked-entries branch from 1c5109c to 2caf63c Compare May 10, 2024 14:58

WillRochaThomas commented May 10, 2024

View reviewed changes

floriank reviewed May 22, 2024

View reviewed changes

address PR feedback

ad77c37

floriank self-requested a review June 24, 2024 17:47

floriank approved these changes Jun 24, 2024

View reviewed changes

floriank merged commit ad8e6e6 into contentful-userland:main Jun 25, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace links in "fields" with linked to entites from "includes" #181

Replace links in "fields" with linked to entites from "includes" #181

WillRochaThomas commented May 6, 2024 •

edited

Loading

WillRochaThomas May 6, 2024

WillRochaThomas May 10, 2024

floriank May 22, 2024

WillRochaThomas May 22, 2024

WillRochaThomas commented May 17, 2024

floriank commented May 17, 2024

floriank left a comment

floriank May 22, 2024

WillRochaThomas May 22, 2024

floriank May 27, 2024

floriank May 22, 2024

WillRochaThomas May 22, 2024

floriank May 27, 2024

floriank May 22, 2024

WillRochaThomas commented May 22, 2024

WillRochaThomas commented May 28, 2024 •

edited

Loading

coveralls commented May 28, 2024

WillRochaThomas commented Jun 20, 2024

floriank commented Jun 20, 2024

WillRochaThomas commented Jun 24, 2024

Replace links in "fields" with linked to entites from "includes" #181

Replace links in "fields" with linked to entites from "includes" #181

Conversation

WillRochaThomas commented May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillRochaThomas commented May 17, 2024

floriank commented May 17, 2024

floriank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillRochaThomas commented May 22, 2024

WillRochaThomas commented May 28, 2024 • edited Loading

coveralls commented May 28, 2024

Pull Request Test Coverage Report for Build ad77c37f1c266f4dfa31049beaedc7eacddb3d4e-PR-181

Details

💛 - Coveralls

WillRochaThomas commented Jun 20, 2024

floriank commented Jun 20, 2024

WillRochaThomas commented Jun 24, 2024

WillRochaThomas commented May 6, 2024 •

edited

Loading

WillRochaThomas commented May 28, 2024 •

edited

Loading