Skip to content

Deserializer only links attributes and not included relationships #497

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

Closed
amit-ch opened this issue Apr 24, 2019 · 5 comments
Closed

Deserializer only links attributes and not included relationships #497

amit-ch opened this issue Apr 24, 2019 · 5 comments

Comments

@amit-ch
Copy link

amit-ch commented Apr 24, 2019

For a simple model like below

    public class Customer : Identifiable
    {
        [HasMany] public List<Order> Orders { get; set; }
    }

    public class Order : Identifiable
    {
        [HasMany] public List<Invoice> Invoices { get; set; }
    }

    public class Invoice : Identifiable { }

Serializer works fine and get customer with included relationship returns the desired response
/api/v1/customers?included=orders.invoices

{
    "data": {
        "attributes": {},
        "relationships": {
            "orders": {
                "links": {
                    "self": "https://localhost:44351/customers/1234/relationships/orders",
                    "related": "https://localhost:44351/customers/1234/orders"
                },
                "data": [
                    {
                        "type": "orders",
                        "id": "4567"
                    }
                ]
            }
        },
        "type": "customers",
        "id": "1234"
    },
    "included": [
        {
            "attributes": {},
            "relationships": {
                "invoices": {
                    "links": {
                        "self": "https://localhost:44351/orders/4567/relationships/invoices",
                        "related": "https://localhost:44351/orders/4567/invoices"
                    },
                    "data": [
                        {
                            "type": "invoices",
                            "id": "7890"
                        }
                    ]
                }
            },
            "type": "orders",
            "id": "4567"
        },
        {
            "attributes": {},
            "type": "invoices",
            "id": "7890"
        }
    ]
}

However on using the same json as input for post/put operation and trying to deserialize into customer instance with all fields/relationships populated, it does not load included relationship.
In this instance, customer has orders attributes populated but invoices is appearing blank.

image

Is it because we are only setting attributes and not relationship. Can we please modify it to add second level or more relationship be included as well.

SetEntityAttributes(relatedInstance, contextEntity, includedResource.Attributes);

@wisepotato
Copy link
Contributor

Can you link to json:api spec where it says this should be supported? Seems like it should.

@maurei
Copy link
Member

maurei commented Apr 25, 2019

I don't think it's a part of the JSON:API spec to support create/update with nested relationships.

If we were to adjust the serialization layer to create the output as you're describing, things will break in the repository layer because the current implementation does not handle updating nested relationships. I don't think we should do that, therefore I think we can close this issue.

However: This issue shows pretty accurately the issue that I have with the current serializers. IMO, the deserializers output should be dependent only on 1) the json input, and 2) the resource graph, and not on any query params (and therefore no dependency on JsonApiContext). Similarly the serializer should depend only on the dataset (which is to be returned) and the resource graph. A (de)serialization layer as such would indeed allow for deserialization of nested relationships like you're describing. But in that case we still need to make sure we're not allowing for create/update of nested relationships.

Rewriting the serializer as such is mentioned in #263 and #253

@amit-ch
Copy link
Author

amit-ch commented Apr 25, 2019

There is no mention on json:api for this so not sure. However I have seen other libraries do that which makes me think this is very much standard.
I plan to use the library without any persistent layer and have my own implementation in service layer.
I think it should be deserializing whatever is available in json input and not create/update nested relationships

@maurei
Copy link
Member

maurei commented Apr 25, 2019

I think it should be deserializing whatever is available in json input and not create/update nested relationships

I totally agree, I'm in!

Work that will be involved:

  1. Serialization layer needs to be decoupled (related to How to use it with n-tier architecture #263, RFC: Separate Concerns of JsonApiContext #253): this will affect JsonApiContext , (in particular, I think, the exposure of HttpContextAccessor) so that we can decouple it from query params.
  2. Need to prevent nested relationship create/update in default implementation of repository/service layer
  3. Need to investigate impact on other components of the framework
  4. Lots of rewriting of tests of serializers where mocks of JsonApiContext are used

This will surely involve breaking changes in the API. I hope I can make a proposal where to start on soon. If you feel like getting involved, we would greatly encourage that!

@maurei
Copy link
Member

maurei commented Oct 10, 2019

resolved in #558

@maurei maurei closed this as completed Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants