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

Add links to nested content properties #1041

Merged
merged 2 commits into from
Aug 23, 2022

Conversation

vierbergenlars
Copy link
Contributor

Use MappingContext to retrieve all URLs that are registered for content
properties, also for nested properties.


It("should add content property links", () -> {
assertThat(resource.getLinks("child"), hasItems(hasProperty("href", is("http://localhost/contentApi/files/999/child"))));
assertThat(resource.getLinks("child/content"), hasItems(hasProperty("href", is("http://localhost/contentApi/files/999/child/content"))));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not completely sure about this link name, any different suggestions or conventions (child.content?) to name links to nested content properties would be welcome.

Copy link
Owner

Choose a reason for hiding this comment

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

I am not aware of a standard for alps links in this regard. What I am doing here is a little bit unique I believe. So, I think it's up to us?

child/content perhaps reflects how the content property as it is addressed when making the request? i.e. http://localhost:8080/contacts/12345/child/content

child.content reflects how the content is fetched internally using BeanWrapper and property editors.

Assuming these links are fetched on the consumer side and '/' will not cause any issues I think I prefer the former?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want to access it from a javascript frontend, both . and / are equally annoying to deal with (You can't do response._links.child.href, but you will have to use response._links["child/content"].href) But that's just a minor concern.

So I think I'll keep the slashes in the link name, as that leaves us with less conversion work.

@vierbergenlars
Copy link
Contributor Author

Hmm, trying this out in a real application, it looks like ContentLinksResourceProcessor is also called with the nested object as PersistentEntityResource.

I need to add a test for that case as well, and handle when store is unavoidably null in that case.

Use MappingContext to retrieve all URLs that are registered for content
properties, also for nested properties.
@vierbergenlars
Copy link
Contributor Author

I worked those issues out last week, and I did not find any new problems with my test application that is using this branch.

Can you take a look at it when you have some time @paulcwarren ?

@paulcwarren
Copy link
Owner

Looks good to me. I have never particularly liked that the mapping context creates two paths for the same content property. And with this now there will be links in the response too.

These are backward compatible "shortcut links". I could try and move that implementation code to a better place in the code and wrap it in an isShortcut condition; i.e. try and remove the duplication.

From your comments I am guessing your apps will follow the "child/content" link, not the "child" link and you wont care if I do manage to clean up and remove it?

@paulcwarren paulcwarren merged commit e957606 into paulcwarren:main Aug 23, 2022
@vierbergenlars
Copy link
Contributor Author

From your comments I am guessing your apps will follow the "child/content" link, not the "child" link and you wont care if I do manage to clean up and remove it?

I'm not entirely sure yet what links that our apps would follow.

I am looking into using one @Embeddable class Content for every content property, and it would be nice to access those without having to append /content to all the content link names

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.

2 participants