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

No way to force link rel to be an array #153

Closed
kuehlmeyer opened this issue Nov 5, 2019 · 10 comments
Closed

No way to force link rel to be an array #153

kuehlmeyer opened this issue Nov 5, 2019 · 10 comments

Comments

@kuehlmeyer
Copy link
Contributor

The question: Is there any way to force WebApi.HAL to return a link rel with a single link as a JSON array?

Background:
It seems, WebApi.Hal determines at runtime whether it returns a a JSON object or a JSON array for a link rel. If there's a link rel called "test" with one link in a Representation, it returns a JSON object;

{
  "_links": {
    "test": {
      "href": "http://dummy1"
    }
  }
}

If there are two or more links of link rels "test", it returns a JSON array:

{
  "_links": {
    "test": [
      {
        "href": "http://dummy1"
      },
      {
        "href": "http://dummy2"
      }
    ]
  }
}

This doesn't sound right. The HAL spec (http://stateless.co/hal_specification.html) says under "Representing Multiple Links With The Same Relation":
For link relations that may have multiple links, we use an array of links.
That sounds much more as if it shouldn't be a matter of the amount of links at runtime (as WebApi.Hal does), it is rather a matter of the general properties of the link rel itself. If a link rel may have multiple links, it should always return a JSON array (probably with only one entry) and never just a JSON object. The HAL spec continues with:
If you're unsure whether the link should be singular, assume it will be multiple. If you pick singular and find you need to change it, you will need to create a new link relation or face breaking existing clients.
Again, that's a strong indication that it's a matter of the link rel itself (may have multiple links => array).

@kbrichan
Copy link
Collaborator

kbrichan commented Nov 5, 2019

You make a good point. To implement this there would need to be a way to declare intended cardinality. This doesn't exist yet in the Representation or Link classes.
Do you have a proposed solution?

@kuehlmeyer
Copy link
Contributor Author

The change should, of course, be done in a way that doesn't affect existing code at all. How about this:

  1. Add a new boolean property IsMultiLink to the Link class. Default is false, meaning: all works as before.
  2. Change the LinksConverter to always write an array if this property is set for any link with a specific link rel, even if there's only one link.

I added a ZIP file with the source code for that: Multi-link-rels.zip. I don't think I can commit to a branch and create a pull request. Please note that the current csproj file in the repository is broken for WebApi.HAL. I also added a unit test.

Usage example:
var resourceWithAppPath = new OrganisationWithLinkTitleRepresentation(1, "Org Name"); resourceWithAppPath.Links.Add(new Link("multi-rel", "~/api/organisations/test") { IsMultiLink = true });

@kuehlmeyer
Copy link
Contributor Author

I forgot:

  1. Change the ResourceConverter to set the new IsMultiLink property in the Link class if the link rel has a JSON array as value.

Updated ZIP (including updated unit test for parsing):
Multi-link-rels - updated.zip

@kuehlmeyer
Copy link
Contributor Author

kuehlmeyer commented Nov 6, 2019

  1. If a Representation contains a list of embedded resources (=a property that is assignable to IEnumerable<IResource>), then the generated links should also have the IsMultiLink flag set. In other words: If the class has a list or array of embedded resources, then the generated links should always be a JSON array, even if the list has only one embedded resource.

Updated ZIP (including unit test):
Multi-link-rels - updated again.zip

The really bad news here: This is the first part that changes the behavior of WebApi.HAL for existing clients. In fact, it breaks one unit test (one_item_organisation_list_get_json_test).

@kbrichan
Copy link
Collaborator

kbrichan commented Nov 6, 2019

We are not having an issue with this because we're using the WebApi.Hal client to deserialize the hal/json wire format, which merely causes the Representation.Links property to get flattened independent of whether links come over the wire as singles or arrays.

Are your clients not using a hal/json deserializer?

@kuehlmeyer
Copy link
Contributor Author

Our clients should be fine. But even in our company, we are using code in a whole bunch of programming languages with a bunch of different libraries to provide and consume HAL APIs. All of them should be able to cope with it. I mean, we are returning sometimes JSON arrays, sometimes a JSON object for the same link rel right now.

Maybe I'm just overly cautious. There could be some really odd situations where the change breaks a client that doesn't use WebAPI.HAL to parse the response. But yes, I'm 99% sure that this happens only in situations where the client code is already massively flawed (meaning: only accepts a single link where the service is actually built in a way that can result in multiple links of the same link rel). So, it is most probably alright.

@kbrichan
Copy link
Collaborator

kbrichan commented Nov 7, 2019

Your enhancement is very reasonable and one could argue the library should have worked this way from the beginning. It's been a few years since I contributed to this project, so I'm hoping someone more current could take a look at a pull request and make it happen.

kuehlmeyer added a commit to kuehlmeyer/WebApi.Hal that referenced this issue Dec 11, 2019
…lti-link link relation always as an array.

1.) Link class has a new property "IsMultiLink". If true, then the link rel is always rendered as a JSON array, even if there's only one actual link.
2.) When having an enumeration of IResource as property (to represent a list of embedded resources), this also defines the link rel as multi-link link rel, meaning that the links for those embedded resources are also rendered as a JSON array even if there's only one element.
3.) When parsing JSON, the IsMultiLink flag is set depending on the JSON token for a link rel (true for a JSON array, false for a JSON object)
4.) Added unit tests for new functionality. Additionally, the changes result in a different behavior for one unit test (one_item_organisation_list_get_json_test), which was adapted to the new behavior.
@kuehlmeyer
Copy link
Contributor Author

I created a pull request with the proposed changes: #154

@panmanphil
Copy link
Collaborator

@kuehlmeyer it has been a long time, hopefully you are still interested in this. I agree this is a good idea, but it may also be a breaking change for some clients. Your PR already has a minor merge conflict because I also had to fix the weird whitespace issue in the project file. What I'd like to do is make another breaking change, accepting the PR to create a version supporting .net core 3.1 which unfortunately not going to be compatible with netstandard2.0. Ideally we'd put this IsMultiLink change into that work where clients will have to make some code fixes anyway to use, and existing .net framework clients would stick with the current version. Then with .net 5.0 they can all come together again.

@panmanphil
Copy link
Collaborator

This has been updated and merged in the .net core 3.1 update, version 4.0

tobyvhenderson added a commit to tobyvhenderson/WebApi.Hal that referenced this issue Mar 17, 2022
…ti-link link relation always as an array.

1.) Link class has a new property "IsMultiLink". If true, then the link rel is always rendered as a JSON array, even if there's only one actual link.
2.) When having an enumeration of IResource as property (to represent a list of embedded resources), this also defines the link rel as multi-link link rel, meaning that the links for those embedded resources are also rendered as a JSON array even if there's only one element.
3.) When parsing JSON, the IsMultiLink flag is set depending on the JSON token for a link rel (true for a JSON array, false for a JSON object)
4.) Added unit tests for new functionality. Additionally, the changes result in a different behavior for one unit test (one_item_organisation_list_get_json_test), which was adapted to the new behavior.
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

No branches or pull requests

3 participants