Skip to content

Commit 8d887e2

Browse files
author
Bart Koelman
committed
AV1562: Do not use ref/out parameters
1 parent 5068bd7 commit 8d887e2

File tree

4 files changed

+101
-64
lines changed

4 files changed

+101
-64
lines changed

src/JsonApiDotNetCore/Hooks/Internal/Execution/HookExecutorHelper.cs

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,10 @@ public Dictionary<RelationshipAttribute, IEnumerable> LoadImplicitlyAffected(Dic
209209

210210
foreach (KeyValuePair<RelationshipAttribute, IEnumerable> kvp in leftResourcesByRelation)
211211
{
212-
if (IsHasManyThrough(kvp, out IEnumerable lefts, out RelationshipAttribute relationship))
212+
RelationshipAttribute relationship = kvp.Key;
213+
IEnumerable lefts = kvp.Value;
214+
215+
if (relationship is HasManyThroughAttribute)
213216
{
214217
continue;
215218
}
@@ -270,12 +273,5 @@ private static void AddToList(IList list, IEnumerable itemsToAdd)
270273
list.Add(item);
271274
}
272275
}
273-
274-
private bool IsHasManyThrough(KeyValuePair<RelationshipAttribute, IEnumerable> kvp, out IEnumerable resources, out RelationshipAttribute attr)
275-
{
276-
attr = kvp.Key;
277-
resources = kvp.Value;
278-
return kvp.Key is HasManyThroughAttribute;
279-
}
280276
}
281277
}

src/JsonApiDotNetCore/Hooks/Internal/ResourceHookExecutor.cs

Lines changed: 78 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -67,60 +67,69 @@ public void BeforeRead<TResource>(ResourcePipeline pipeline, string stringId = n
6767
public IEnumerable<TResource> BeforeUpdate<TResource>(IEnumerable<TResource> resources, ResourcePipeline pipeline)
6868
where TResource : class, IIdentifiable
6969
{
70-
if (GetHook(ResourceHook.BeforeUpdate, resources, out IResourceHookContainer<TResource> container, out RootNode<TResource> node))
70+
GetHookResult<TResource> result = GetHook(ResourceHook.BeforeUpdate, resources);
71+
72+
if (result.Succeeded)
7173
{
72-
RelationshipAttribute[] relationships = node.RelationshipsToNextLayer.Select(proxy => proxy.Attribute).ToArray();
73-
IEnumerable dbValues = LoadDbValues(typeof(TResource), (IEnumerable<TResource>)node.UniqueResources, ResourceHook.BeforeUpdate, relationships);
74-
var diff = new DiffableResourceHashSet<TResource>(node.UniqueResources, dbValues, node.LeftsToNextLayer(), _targetedFields);
75-
IEnumerable<TResource> updated = container.BeforeUpdate(diff, pipeline);
76-
node.UpdateUnique(updated);
77-
node.Reassign(resources);
74+
RelationshipAttribute[] relationships = result.Node.RelationshipsToNextLayer.Select(proxy => proxy.Attribute).ToArray();
75+
76+
IEnumerable dbValues = LoadDbValues(typeof(TResource), (IEnumerable<TResource>)result.Node.UniqueResources, ResourceHook.BeforeUpdate,
77+
relationships);
78+
79+
var diff = new DiffableResourceHashSet<TResource>(result.Node.UniqueResources, dbValues, result.Node.LeftsToNextLayer(), _targetedFields);
80+
IEnumerable<TResource> updated = result.Container.BeforeUpdate(diff, pipeline);
81+
result.Node.UpdateUnique(updated);
82+
result.Node.Reassign(resources);
7883
}
7984

80-
FireNestedBeforeUpdateHooks(pipeline, _traversalHelper.CreateNextLayer(node));
85+
FireNestedBeforeUpdateHooks(pipeline, _traversalHelper.CreateNextLayer(result.Node));
8186
return resources;
8287
}
8388

8489
/// <inheritdoc />
8590
public IEnumerable<TResource> BeforeCreate<TResource>(IEnumerable<TResource> resources, ResourcePipeline pipeline)
8691
where TResource : class, IIdentifiable
8792
{
88-
if (GetHook(ResourceHook.BeforeCreate, resources, out IResourceHookContainer<TResource> container, out RootNode<TResource> node))
93+
GetHookResult<TResource> result = GetHook(ResourceHook.BeforeCreate, resources);
94+
95+
if (result.Succeeded)
8996
{
90-
var affected = new ResourceHashSet<TResource>((HashSet<TResource>)node.UniqueResources, node.LeftsToNextLayer());
91-
IEnumerable<TResource> updated = container.BeforeCreate(affected, pipeline);
92-
node.UpdateUnique(updated);
93-
node.Reassign(resources);
97+
var affected = new ResourceHashSet<TResource>((HashSet<TResource>)result.Node.UniqueResources, result.Node.LeftsToNextLayer());
98+
IEnumerable<TResource> updated = result.Container.BeforeCreate(affected, pipeline);
99+
result.Node.UpdateUnique(updated);
100+
result.Node.Reassign(resources);
94101
}
95102

96-
FireNestedBeforeUpdateHooks(pipeline, _traversalHelper.CreateNextLayer(node));
103+
FireNestedBeforeUpdateHooks(pipeline, _traversalHelper.CreateNextLayer(result.Node));
97104
return resources;
98105
}
99106

100107
/// <inheritdoc />
101108
public IEnumerable<TResource> BeforeDelete<TResource>(IEnumerable<TResource> resources, ResourcePipeline pipeline)
102109
where TResource : class, IIdentifiable
103110
{
104-
if (GetHook(ResourceHook.BeforeDelete, resources, out IResourceHookContainer<TResource> container, out RootNode<TResource> node))
111+
GetHookResult<TResource> result = GetHook(ResourceHook.BeforeDelete, resources);
112+
113+
if (result.Succeeded)
105114
{
106-
RelationshipAttribute[] relationships = node.RelationshipsToNextLayer.Select(proxy => proxy.Attribute).ToArray();
115+
RelationshipAttribute[] relationships = result.Node.RelationshipsToNextLayer.Select(proxy => proxy.Attribute).ToArray();
107116

108117
IEnumerable targetResources =
109-
LoadDbValues(typeof(TResource), (IEnumerable<TResource>)node.UniqueResources, ResourceHook.BeforeDelete, relationships) ??
110-
node.UniqueResources;
118+
LoadDbValues(typeof(TResource), (IEnumerable<TResource>)result.Node.UniqueResources, ResourceHook.BeforeDelete, relationships) ??
119+
result.Node.UniqueResources;
111120

112-
var affected = new ResourceHashSet<TResource>(targetResources, node.LeftsToNextLayer());
121+
var affected = new ResourceHashSet<TResource>(targetResources, result.Node.LeftsToNextLayer());
113122

114-
IEnumerable<TResource> updated = container.BeforeDelete(affected, pipeline);
115-
node.UpdateUnique(updated);
116-
node.Reassign(resources);
123+
IEnumerable<TResource> updated = result.Container.BeforeDelete(affected, pipeline);
124+
result.Node.UpdateUnique(updated);
125+
result.Node.Reassign(resources);
117126
}
118127

119128
// If we're deleting an article, we're implicitly affected any owners related to it.
120129
// Here we're loading all relations onto the to-be-deleted article
121130
// if for that relation the BeforeImplicitUpdateHook is implemented,
122131
// and this hook is then executed
123-
foreach (KeyValuePair<Type, Dictionary<RelationshipAttribute, IEnumerable>> entry in node.LeftsToNextLayerByRelationships())
132+
foreach (KeyValuePair<Type, Dictionary<RelationshipAttribute, IEnumerable>> entry in result.Node.LeftsToNextLayerByRelationships())
124133
{
125134
Type rightType = entry.Key;
126135
Dictionary<RelationshipAttribute, IEnumerable> implicitTargets = entry.Value;
@@ -134,15 +143,17 @@ public IEnumerable<TResource> BeforeDelete<TResource>(IEnumerable<TResource> res
134143
public IEnumerable<TResource> OnReturn<TResource>(IEnumerable<TResource> resources, ResourcePipeline pipeline)
135144
where TResource : class, IIdentifiable
136145
{
137-
if (GetHook(ResourceHook.OnReturn, resources, out IResourceHookContainer<TResource> container, out RootNode<TResource> node))
146+
GetHookResult<TResource> result = GetHook(ResourceHook.OnReturn, resources);
147+
148+
if (result.Succeeded)
138149
{
139-
IEnumerable<TResource> updated = container.OnReturn((HashSet<TResource>)node.UniqueResources, pipeline);
150+
IEnumerable<TResource> updated = result.Container.OnReturn((HashSet<TResource>)result.Node.UniqueResources, pipeline);
140151
ValidateHookResponse(updated);
141-
node.UpdateUnique(updated);
142-
node.Reassign(resources);
152+
result.Node.UpdateUnique(updated);
153+
result.Node.Reassign(resources);
143154
}
144155

145-
Traverse(_traversalHelper.CreateNextLayer(node), ResourceHook.OnReturn, (nextContainer, nextNode) =>
156+
Traverse(_traversalHelper.CreateNextLayer(result.Node), ResourceHook.OnReturn, (nextContainer, nextNode) =>
146157
{
147158
IEnumerable filteredUniqueSet = CallHook(nextContainer, ResourceHook.OnReturn, ArrayFactory.Create<object>(nextNode.UniqueResources, pipeline));
148159
nextNode.UpdateUnique(filteredUniqueSet);
@@ -156,12 +167,14 @@ public IEnumerable<TResource> OnReturn<TResource>(IEnumerable<TResource> resourc
156167
public void AfterRead<TResource>(IEnumerable<TResource> resources, ResourcePipeline pipeline)
157168
where TResource : class, IIdentifiable
158169
{
159-
if (GetHook(ResourceHook.AfterRead, resources, out IResourceHookContainer<TResource> container, out RootNode<TResource> node))
170+
GetHookResult<TResource> result = GetHook(ResourceHook.AfterRead, resources);
171+
172+
if (result.Succeeded)
160173
{
161-
container.AfterRead((HashSet<TResource>)node.UniqueResources, pipeline);
174+
result.Container.AfterRead((HashSet<TResource>)result.Node.UniqueResources, pipeline);
162175
}
163176

164-
Traverse(_traversalHelper.CreateNextLayer(node), ResourceHook.AfterRead, (nextContainer, nextNode) =>
177+
Traverse(_traversalHelper.CreateNextLayer(result.Node), ResourceHook.AfterRead, (nextContainer, nextNode) =>
165178
{
166179
CallHook(nextContainer, ResourceHook.AfterRead, ArrayFactory.Create<object>(nextNode.UniqueResources, pipeline, true));
167180
});
@@ -171,35 +184,41 @@ public void AfterRead<TResource>(IEnumerable<TResource> resources, ResourcePipel
171184
public void AfterCreate<TResource>(IEnumerable<TResource> resources, ResourcePipeline pipeline)
172185
where TResource : class, IIdentifiable
173186
{
174-
if (GetHook(ResourceHook.AfterCreate, resources, out IResourceHookContainer<TResource> container, out RootNode<TResource> node))
187+
GetHookResult<TResource> result = GetHook(ResourceHook.AfterCreate, resources);
188+
189+
if (result.Succeeded)
175190
{
176-
container.AfterCreate((HashSet<TResource>)node.UniqueResources, pipeline);
191+
result.Container.AfterCreate((HashSet<TResource>)result.Node.UniqueResources, pipeline);
177192
}
178193

179-
Traverse(_traversalHelper.CreateNextLayer(node), ResourceHook.AfterUpdateRelationship,
194+
Traverse(_traversalHelper.CreateNextLayer(result.Node), ResourceHook.AfterUpdateRelationship,
180195
(nextContainer, nextNode) => FireAfterUpdateRelationship(nextContainer, nextNode, pipeline));
181196
}
182197

183198
/// <inheritdoc />
184199
public void AfterUpdate<TResource>(IEnumerable<TResource> resources, ResourcePipeline pipeline)
185200
where TResource : class, IIdentifiable
186201
{
187-
if (GetHook(ResourceHook.AfterUpdate, resources, out IResourceHookContainer<TResource> container, out RootNode<TResource> node))
202+
GetHookResult<TResource> result = GetHook(ResourceHook.AfterUpdate, resources);
203+
204+
if (result.Succeeded)
188205
{
189-
container.AfterUpdate((HashSet<TResource>)node.UniqueResources, pipeline);
206+
result.Container.AfterUpdate((HashSet<TResource>)result.Node.UniqueResources, pipeline);
190207
}
191208

192-
Traverse(_traversalHelper.CreateNextLayer(node), ResourceHook.AfterUpdateRelationship,
209+
Traverse(_traversalHelper.CreateNextLayer(result.Node), ResourceHook.AfterUpdateRelationship,
193210
(nextContainer, nextNode) => FireAfterUpdateRelationship(nextContainer, nextNode, pipeline));
194211
}
195212

196213
/// <inheritdoc />
197214
public void AfterDelete<TResource>(IEnumerable<TResource> resources, ResourcePipeline pipeline, bool succeeded)
198215
where TResource : class, IIdentifiable
199216
{
200-
if (GetHook(ResourceHook.AfterDelete, resources, out IResourceHookContainer<TResource> container, out RootNode<TResource> node))
217+
GetHookResult<TResource> result = GetHook(ResourceHook.AfterDelete, resources);
218+
219+
if (result.Succeeded)
201220
{
202-
container.AfterDelete((HashSet<TResource>)node.UniqueResources, pipeline, succeeded);
221+
result.Container.AfterDelete((HashSet<TResource>)result.Node.UniqueResources, pipeline, succeeded);
203222
}
204223
}
205224

@@ -212,13 +231,13 @@ public void AfterDelete<TResource>(IEnumerable<TResource> resources, ResourcePip
212231
/// <returns>
213232
/// <c>true</c>, if hook was implemented, <c>false</c> otherwise.
214233
/// </returns>
215-
private bool GetHook<TResource>(ResourceHook target, IEnumerable<TResource> resources, out IResourceHookContainer<TResource> container,
216-
out RootNode<TResource> node)
234+
private GetHookResult<TResource> GetHook<TResource>(ResourceHook target, IEnumerable<TResource> resources)
217235
where TResource : class, IIdentifiable
218236
{
219-
node = _traversalHelper.CreateRootNode(resources);
220-
container = _executorHelper.GetResourceHookContainer<TResource>(target);
221-
return container != null;
237+
RootNode<TResource> node = _traversalHelper.CreateRootNode(resources);
238+
IResourceHookContainer<TResource> container = _executorHelper.GetResourceHookContainer<TResource>(target);
239+
240+
return new GetHookResult<TResource>(container, node);
222241
}
223242

224243
/// <summary>
@@ -577,5 +596,20 @@ private HashSet<string> GetIds(IEnumerable resources)
577596
{
578597
return new HashSet<string>(resources.Cast<IIdentifiable>().Select(resource => resource.StringId));
579598
}
599+
600+
private sealed class GetHookResult<TResource>
601+
where TResource : class, IIdentifiable
602+
{
603+
public IResourceHookContainer<TResource> Container { get; }
604+
public RootNode<TResource> Node { get; }
605+
606+
public bool Succeeded => Container != null;
607+
608+
public GetHookResult(IResourceHookContainer<TResource> container, RootNode<TResource> node)
609+
{
610+
Container = container;
611+
Node = node;
612+
}
613+
}
580614
}
581615
}

src/JsonApiDotNetCore/Serialization/Building/ResponseResourceObjectBuilder.cs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,13 +70,13 @@ protected override RelationshipEntry GetRelationshipData(RelationshipAttribute r
7070
ArgumentGuard.NotNull(resource, nameof(resource));
7171

7272
RelationshipEntry relationshipEntry = null;
73-
List<IReadOnlyCollection<RelationshipAttribute>> relationshipChains = null;
73+
IReadOnlyCollection<IReadOnlyCollection<RelationshipAttribute>> relationshipChains = GetInclusionChain(relationship);
7474

75-
if (Equals(relationship, _requestRelationship) || ShouldInclude(relationship, out relationshipChains))
75+
if (Equals(relationship, _requestRelationship) || relationshipChains.Any())
7676
{
7777
relationshipEntry = base.GetRelationshipData(relationship, resource);
7878

79-
if (relationshipChains != null && relationshipEntry.HasResource)
79+
if (relationshipChains.Any() && relationshipEntry.HasResource)
8080
{
8181
foreach (IReadOnlyCollection<RelationshipAttribute> chain in relationshipChains)
8282
{
@@ -117,7 +117,7 @@ private bool IsRelationshipInSparseFieldSet(RelationshipAttribute relationship)
117117
/// Inspects the included relationship chains (see <see cref="IIncludeQueryStringParameterReader" /> to see if <paramref name="relationship" /> should be
118118
/// included or not.
119119
/// </summary>
120-
private bool ShouldInclude(RelationshipAttribute relationship, out List<IReadOnlyCollection<RelationshipAttribute>> inclusionChain)
120+
private IReadOnlyCollection<IReadOnlyCollection<RelationshipAttribute>> GetInclusionChain(RelationshipAttribute relationship)
121121
{
122122
// @formatter:wrap_chained_method_calls chop_always
123123
// @formatter:keep_existing_linebreaks true
@@ -132,7 +132,7 @@ private bool ShouldInclude(RelationshipAttribute relationship, out List<IReadOnl
132132
// @formatter:keep_existing_linebreaks restore
133133
// @formatter:wrap_chained_method_calls restore
134134

135-
inclusionChain = new List<IReadOnlyCollection<RelationshipAttribute>>();
135+
var inclusionChain = new List<IReadOnlyCollection<RelationshipAttribute>>();
136136

137137
foreach (ResourceFieldChainExpression chain in chains)
138138
{
@@ -142,7 +142,7 @@ private bool ShouldInclude(RelationshipAttribute relationship, out List<IReadOnl
142142
}
143143
}
144144

145-
return inclusionChain.Any();
145+
return inclusionChain;
146146
}
147147
}
148148
}

test/UnitTests/Serialization/Server/RequestDeserializerTests.cs

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,10 @@ public RequestDeserializerTests()
2828
public void DeserializeAttributes_VariousUpdatedMembers_RegistersTargetedFields()
2929
{
3030
// Arrange
31-
SetupFieldsManager(out HashSet<AttrAttribute> attributesToUpdate, out HashSet<RelationshipAttribute> relationshipsToUpdate);
31+
var attributesToUpdate = new HashSet<AttrAttribute>();
32+
var relationshipsToUpdate = new HashSet<RelationshipAttribute>();
33+
SetupFieldsManager(attributesToUpdate, relationshipsToUpdate);
34+
3235
Document content = CreateTestResourceDocument();
3336
string body = JsonConvert.SerializeObject(content);
3437

@@ -44,7 +47,10 @@ public void DeserializeAttributes_VariousUpdatedMembers_RegistersTargetedFields(
4447
public void DeserializeRelationships_MultipleDependentRelationships_RegistersUpdatedRelationships()
4548
{
4649
// Arrange
47-
SetupFieldsManager(out HashSet<AttrAttribute> attributesToUpdate, out HashSet<RelationshipAttribute> relationshipsToUpdate);
50+
var attributesToUpdate = new HashSet<AttrAttribute>();
51+
var relationshipsToUpdate = new HashSet<RelationshipAttribute>();
52+
SetupFieldsManager(attributesToUpdate, relationshipsToUpdate);
53+
4854
Document content = CreateDocumentWithRelationships("multiPrincipals");
4955
content.SingleData.Relationships.Add("populatedToOne", CreateRelationshipData("oneToOneDependents"));
5056
content.SingleData.Relationships.Add("emptyToOne", CreateRelationshipData());
@@ -64,7 +70,10 @@ public void DeserializeRelationships_MultipleDependentRelationships_RegistersUpd
6470
public void DeserializeRelationships_MultiplePrincipalRelationships_RegistersUpdatedRelationships()
6571
{
6672
// Arrange
67-
SetupFieldsManager(out HashSet<AttrAttribute> attributesToUpdate, out HashSet<RelationshipAttribute> relationshipsToUpdate);
73+
var attributesToUpdate = new HashSet<AttrAttribute>();
74+
var relationshipsToUpdate = new HashSet<RelationshipAttribute>();
75+
SetupFieldsManager(attributesToUpdate, relationshipsToUpdate);
76+
6877
Document content = CreateDocumentWithRelationships("multiDependents");
6978
content.SingleData.Relationships.Add("populatedToOne", CreateRelationshipData("oneToOnePrincipals"));
7079
content.SingleData.Relationships.Add("emptyToOne", CreateRelationshipData());
@@ -80,10 +89,8 @@ public void DeserializeRelationships_MultiplePrincipalRelationships_RegistersUpd
8089
Assert.Empty(attributesToUpdate);
8190
}
8291

83-
private void SetupFieldsManager(out HashSet<AttrAttribute> attributesToUpdate, out HashSet<RelationshipAttribute> relationshipsToUpdate)
92+
private void SetupFieldsManager(HashSet<AttrAttribute> attributesToUpdate, HashSet<RelationshipAttribute> relationshipsToUpdate)
8493
{
85-
attributesToUpdate = new HashSet<AttrAttribute>();
86-
relationshipsToUpdate = new HashSet<RelationshipAttribute>();
8794
_fieldsManagerMock.Setup(fields => fields.Attributes).Returns(attributesToUpdate);
8895
_fieldsManagerMock.Setup(fields => fields.Relationships).Returns(relationshipsToUpdate);
8996
}

0 commit comments

Comments
 (0)