Optional links support #153

bjornharrtell · 2017-05-17T20:09:44Z

Aims to resolve #150.

Not yet finished though but wanted to ask about the approach. I couldn't see it possible to handle this in the UrlPathBuilder but had to involve the ResourceSerializer. Also, I'm not sure about what a "canonical" url is and so have problems understanding the purpose of Related vs RelatedCanonical.

joukevandermaas

I think this approach makes sense. I originally thought the UrlPathBuilder could return null based on the resource, then the serializer could omit the links hash if empty. I think your approach is more clear.

I do think we need some more tests, at least one for each of the enum values.

joukevandermaas · 2017-05-18T07:36:28Z

Saule/ApiResource.cs

@@ -58,6 +68,8 @@ protected ApiResource()
        /// </summary>
        public string IdProperty { get; private set; }

+        public LinkTypes WithLinks { get; set; } = LinkTypes.All;


I don't think ApiResource should have any mutable properties.

joukevandermaas · 2017-05-18T07:43:55Z

Saule/Serialization/ResourceSerializer.cs

-            var result = new JObject
+            var result = new JObject();
+
+            if (_resource.WithLinks.HasFlag(LinkTypes.Self & LinkTypes.All))


I think you can just do HasFlag(LinkTypes.Self), since All includes that. Now this is only added when the value is LinkTypes.All.

joukevandermaas · 2017-05-18T07:44:42Z

Saule/Serialization/ResourceSerializer.cs


-                item["links"] = links;
+                if (!string.IsNullOrEmpty(self) && relationship.WithLinks.HasFlag(LinkTypes.Related & LinkTypes.All))


Same here, you don't need to do the &.

joukevandermaas · 2017-05-18T07:44:48Z

Saule/Serialization/ResourceSerializer.cs

+                    AddUrl(links, "self", self);
+                }
+
+                if (!string.IsNullOrEmpty(related) && relationship.WithLinks.HasFlag(LinkTypes.Related & LinkTypes.All))


bjornharrtell · 2017-05-18T07:56:50Z

Thanks @joukevandermaas, good to know I'm hopefully on the right path. Will ping you when I have had the time to make this PR complete and properly reviewable and will take your early comments into consideration.

bjornharrtell · 2017-05-18T20:51:32Z

Improved now but amount of tests still too few and should try it out on a real API implementation too.

bjornharrtell · 2017-05-19T20:35:51Z

Additional tests added and seems to do what I expect in a real project. Ready for review now @joukevandermaas.

But... I did notice a possible regression with the new serializer. Using 1.7.0.390 I'm missing the data property for relationships (only tested BelongsTo as of yet), downgrading to stable 1.6 resolves it.

laurence79 · 2017-05-20T07:04:51Z

@bjornharrtell damn... could you give me an example or even better, a test, and I'll investigate the regression?

bjornharrtell · 2017-05-20T10:09:38Z

@laurence79 will try to produce a unit test to demonstrate the issue.

bjornharrtell · 2017-05-20T12:04:00Z

See testcase in #155.

joukevandermaas

I think there's a slight confusion because both resources and relationships can define links.

This also needs documentation because of new public API.

joukevandermaas · 2017-05-22T06:17:16Z

Saule/Serialization/ResourceSerializer.cs


-                item["links"] = links;
+                if (!string.IsNullOrEmpty(self) && relationship.LinkType.HasFlag(LinkType.Self))


I think for resources, we should both check their resources LinkType as well as the relationship's LinkType. It should probably resolve to the most restricting value.

Exampe:

PersonResource: WithLinks(All) - BelongsTo<CompanyResource> Job, WithLinks(None) CompanyResource: WithLinks(Self)

Now when serializing the Job relationship, what links should show up? If I read this code correctly, currently it will serialize the self link even if the BelongsTo specified not to.

We probably also need a test case for this.

Good question, I have a hard time getting a grip on what would be correct/desired behaviour here. Will have to think about it some more.

I wrote down some thoughts on this below. Please let me know what you think.

It's most likely that one of the two will just be All. I expect the most common scenario is to just override something in one place (i.e. either the relationship or on the model, depending which best maps to the task.

This implies to me that we should take the most restrictive of the options. Examples:

Person -> Company (Job relationship). Person and company both have link type All, the relationship has link type Related. When serializing individual person or company records, we should serialize all links. When serializing the job relationship, we should serialize only related links.

Person -> Company (Job relationship). Person has link type All, company has Related. The relationship has link type All. When serializing individual person records, we should serialize all links. For company records, we should not serialize anything (because there's no related links in that case). For the job relationship, we should serialize only related links.

One case where I'm not sure:

Person -> Company (Job relationship). Person and company both have link type None. The relationship has link type Related. When serializing individual records, we should serialize no links. For the job relationship, what should we do? It sort of makes sense to serialize no links, but then again, the user explicitly set the link type to Related so they probably want those.

I guess the solution would be something like this:

When not serializing relationships, there is only one option so this should be simple.

Otherwise, if one of the options is All, take the other option.

In all other cases, use the logical 'or' of the options.

Examples of this:

All and None -> serialize none

All and Related -> serialize related

Related and None -> serialize none

Related and Self -> serialize related & self only (where they make sense)

Of course, we always have the option to just ignore one of the two:

When serializing the related hash, use the relationship link type

When serializing models (i.e. the top level data or included hash), use the resource link type

This is simple (I believe this is what the code in this PR does now) and seems the easiest to document.

I'm leaning on keeping it simple. The more complex path still hurts my brain. If one has other needs it could be implemented as a custom UrlBuilder. That said I might be shooting myself in the foot here. What is your opinion?

Yeah, I tend to agree. We can always expand on it later.

joukevandermaas · 2017-05-22T06:17:42Z

Saule/Serialization/ResourceSerializer.cs

                    && kv.Value.SourceObject != null)
                {
                    relationshipId = (string)data["id"];
                }

                var links = new JObject();
-                AddUrl(links, "self", _urlBuilder.BuildRelationshipPath(node.Resource, node.Key.Id.ToString(), kv.Value.Relationship));
-                AddUrl(links, "related", _urlBuilder.BuildRelationshipPath(node.Resource, node.Key.Id.ToString(), kv.Value.Relationship, relationshipId));
+                var self = _urlBuilder.BuildRelationshipPath(node.Resource, node.Key.Id.ToString(), relationship);


We don't need to generate these if we might not use them, I think they should go inside the ifs below.

Needed because I check for empty link generation. I.e unrelated to the LinkType configuration I want to avoid generating the links if the UrlBuilder returns null which I think is in line what you once wrote in #121?

joukevandermaas · 2017-05-29T12:55:20Z

I like the changes! I think this can be merged once the documentation has been updated (I think docs/generating-links.md is a good spot).

bjornharrtell · 2017-05-29T16:33:08Z

Great, will add docs/generating-links.md asap. But what about RelatedCanonical? Shall I remove that one?

bjornharrtell · 2017-05-29T16:51:21Z

Added documentation and tentatively removed RelatedCanonical enum type.

joukevandermaas

The markdown parser used by GH pages is different, and IIRC there needs to be a new line before code blocks.

joukevandermaas · 2017-05-30T07:08:23Z

docs/Generating-links.md

+```
+
+When defining a `Relationship`:
+```csharp


I think there needs to be a new line before a code block or the parser messes up the layout.

joukevandermaas · 2017-05-30T07:08:26Z

docs/Generating-links.md

+specific `ApiResource` or `Relationship` by setting it as in the following examples:
+
+In your `ApiResource` constructor:
+```csharp


I think there needs to be a new line before a code block or the parser messes up the layout.

joukevandermaas · 2017-05-30T08:08:36Z

Thanks for seeing this through! It turned out really nice, I think.

joukevandermaas requested changes May 18, 2017

View reviewed changes

Work in progress optional links support

c38f6ee

bjornharrtell force-pushed the optional-links branch from db31d2e to c38f6ee Compare May 18, 2017 20:48

bjornharrtell changed the title ~~WIP Optional links support~~ Optional links support May 18, 2017

Additional tests

ae36c08

joukevandermaas requested changes May 22, 2017

View reviewed changes

joukevandermaas approved these changes May 29, 2017

View reviewed changes

bjornharrtell added 2 commits May 29, 2017 18:49

Add documentation

003394a

Remove RelatedCanonical

0ae6dcf

joukevandermaas reviewed May 30, 2017

View reviewed changes

Added newlines

3d45a8b

joukevandermaas merged commit bba318e into joukevandermaas:master May 30, 2017

bjornharrtell deleted the optional-links branch May 30, 2017 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional links support #153

Optional links support #153

bjornharrtell commented May 17, 2017

joukevandermaas left a comment

joukevandermaas May 18, 2017

joukevandermaas May 18, 2017

joukevandermaas May 18, 2017

joukevandermaas May 18, 2017

bjornharrtell commented May 18, 2017

bjornharrtell commented May 18, 2017 •

edited

Loading

bjornharrtell commented May 19, 2017

laurence79 commented May 20, 2017

bjornharrtell commented May 20, 2017

bjornharrtell commented May 20, 2017

joukevandermaas left a comment

joukevandermaas May 22, 2017

bjornharrtell May 24, 2017

joukevandermaas May 26, 2017

bjornharrtell May 29, 2017

joukevandermaas May 29, 2017

joukevandermaas May 22, 2017

bjornharrtell May 24, 2017

joukevandermaas commented May 29, 2017

bjornharrtell commented May 29, 2017

bjornharrtell commented May 29, 2017

joukevandermaas left a comment

joukevandermaas May 30, 2017

joukevandermaas May 30, 2017

joukevandermaas commented May 30, 2017


		item["links"] = links;
		if (!string.IsNullOrEmpty(self) && relationship.WithLinks.HasFlag(LinkTypes.Related & LinkTypes.All))

Optional links support #153

Optional links support #153

Conversation

bjornharrtell commented May 17, 2017

joukevandermaas 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

bjornharrtell commented May 18, 2017

bjornharrtell commented May 18, 2017 • edited Loading

bjornharrtell commented May 19, 2017

laurence79 commented May 20, 2017

bjornharrtell commented May 20, 2017

bjornharrtell commented May 20, 2017

joukevandermaas 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

joukevandermaas commented May 29, 2017

bjornharrtell commented May 29, 2017

bjornharrtell commented May 29, 2017

joukevandermaas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joukevandermaas commented May 30, 2017

bjornharrtell commented May 18, 2017 •

edited

Loading