Skip to content

Commit 8a6357a

Browse files
andrewlockandrewlock
committed
Fix routing bugs where values are missing
1 parent 509fe68 commit 8a6357a

File tree

2 files changed

+77
-55
lines changed

2 files changed

+77
-55
lines changed

‎tracer/src/Datadog.Trace/DiagnosticListeners/AspNetCoreResourceNameHelper.cs‎

Lines changed: 71 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -47,73 +47,53 @@ internal static string SimplifyRoutePattern(
4747
else if (part.TryDuckCast(out AspNetCoreDiagnosticObserver.RoutePatternParameterPartStruct parameter))
4848
{
4949
var parameterName = parameter.Name;
50-
if (parameterName.Equals("area", StringComparison.OrdinalIgnoreCase)
51-
|| parameterName.Equals("controller", StringComparison.OrdinalIgnoreCase)
52-
|| parameterName.Equals("action", StringComparison.OrdinalIgnoreCase))
50+
var haveParameter = routeValueDictionary.TryGetValue(parameterName, out var value);
51+
if (!parameter.IsOptional || haveParameter)
5352
{
5453
if (!addedPart)
5554
{
5655
sb.Append('/');
5756
addedPart = true;
5857
}
5958

60-
if (routeValueDictionary.TryGetValue(parameterName, out var value)
61-
&& value is string name)
59+
// Is this parameter an identifier segment? we assume non-strings _are_ identifiers
60+
// so never expand them. This avoids an allocating ToString() call, but means that
61+
// some parameters which maybe _should_ be expanded (e.g. Enum)s currently are not
62+
if (haveParameter
63+
&& (expandRouteParameters
64+
|| parameterName.Equals("area", StringComparison.OrdinalIgnoreCase)
65+
|| parameterName.Equals("controller", StringComparison.OrdinalIgnoreCase)
66+
|| parameterName.Equals("action", StringComparison.OrdinalIgnoreCase))
67+
&& (value is null ||
68+
(value is string valueAsString
69+
&& !UriHelpers.IsIdentifierSegment(valueAsString, 0, valueAsString.Length))))
6270
{
63-
sb.AppendAsLowerInvariant(name);
71+
// write the expanded parameter value
72+
sb.AppendAsLowerInvariant(value as string);
6473
}
6574
else
6675
{
67-
sb.Append(parameterName);
68-
}
69-
}
70-
else
71-
{
72-
var haveParameter = routeValueDictionary.TryGetValue(parameterName, out var value);
73-
if (!parameter.IsOptional || haveParameter)
74-
{
75-
if (!addedPart)
76-
{
77-
sb.Append('/');
78-
addedPart = true;
79-
}
80-
81-
// Is this parameter an identifier segment? we assume non-strings _are_ identifiers
82-
// so never expand them. This avoids an allocating ToString() call, but means that
83-
// some parameters which maybe _should_ be expanded (e.g. Enum)s currently are not
84-
if (expandRouteParameters
85-
&& haveParameter
86-
&& (value is null ||
87-
(value is string valueAsString
88-
&& !UriHelpers.IsIdentifierSegment(valueAsString, 0, valueAsString.Length))))
89-
{
90-
// write the expanded parameter value
91-
sb.AppendAsLowerInvariant(value as string);
92-
}
93-
else
76+
// write the route template value
77+
sb.Append('{');
78+
if (parameter.IsCatchAll)
9479
{
95-
// write the route template value
96-
sb.Append('{');
97-
if (parameter.IsCatchAll)
80+
if (parameter.EncodeSlashes)
9881
{
99-
if (parameter.EncodeSlashes)
100-
{
101-
sb.Append("**");
102-
}
103-
else
104-
{
105-
sb.Append('*');
106-
}
82+
sb.Append("**");
10783
}
108-
109-
sb.AppendAsLowerInvariant(parameterName);
110-
if (parameter.IsOptional)
84+
else
11185
{
112-
sb.Append('?');
86+
sb.Append('*');
11387
}
88+
}
11489

115-
sb.Append('}');
90+
sb.AppendAsLowerInvariant(parameterName);
91+
if (parameter.IsOptional)
92+
{
93+
sb.Append('?');
11694
}
95+
96+
sb.Append('}');
11797
}
11898
}
11999
}
@@ -167,30 +147,48 @@ internal static string SimplifyRoutePattern(
167147
var parameterName = parameter.Name;
168148
if (parameterName.Equals("area", StringComparison.OrdinalIgnoreCase))
169149
{
150+
if (areaName is null && parameter.IsOptional)
151+
{
152+
// don't append optional suffixes when no value is provided
153+
continue;
154+
}
155+
170156
if (parts == 1)
171157
{
172158
sb.Append('/');
173159
}
174160

175-
sb.Append(areaName);
161+
sb.Append(areaName ?? "{area}");
176162
}
177163
else if (parameterName.Equals("controller", StringComparison.OrdinalIgnoreCase))
178164
{
165+
if (controllerName is null && parameter.IsOptional)
166+
{
167+
// don't append optional suffixes when no value is provided
168+
continue;
169+
}
170+
179171
if (parts == 1)
180172
{
181173
sb.Append('/');
182174
}
183175

184-
sb.Append(controllerName);
176+
sb.Append(controllerName ?? "{controller}");
185177
}
186178
else if (parameterName.Equals("action", StringComparison.OrdinalIgnoreCase))
187179
{
180+
if (actionName is null && parameter.IsOptional)
181+
{
182+
// don't append optional suffixes when no value is provided
183+
continue;
184+
}
185+
188186
if (parts == 1)
189187
{
190188
sb.Append('/');
191189
}
192190

193-
sb.Append(actionName);
191+
sb.Append(actionName ?? "{action}");
194192
}
195193
else
196194
{
@@ -277,30 +275,48 @@ internal static string SimplifyRouteTemplate(
277275
}
278276
else if (partName.Equals("area", StringComparison.OrdinalIgnoreCase))
279277
{
278+
if (areaName is null && part.IsOptional)
279+
{
280+
// don't append optional suffixes when no value is provided
281+
continue;
282+
}
283+
280284
if (parts == 1)
281285
{
282286
sb.Append('/');
283287
}
284288

285-
sb.Append(areaName);
289+
sb.Append(areaName ?? "{area}");
286290
}
287291
else if (partName.Equals("controller", StringComparison.OrdinalIgnoreCase))
288292
{
293+
if (controllerName is null && part.IsOptional)
294+
{
295+
// don't append optional suffixes when no value is provided
296+
continue;
297+
}
298+
289299
if (parts == 1)
290300
{
291301
sb.Append('/');
292302
}
293303

294-
sb.Append(controllerName);
304+
sb.Append(controllerName ?? "{controller}");
295305
}
296306
else if (partName.Equals("action", StringComparison.OrdinalIgnoreCase))
297307
{
308+
if (actionName is null && part.IsOptional)
309+
{
310+
// don't append optional suffixes when no value is provided
311+
continue;
312+
}
313+
298314
if (parts == 1)
299315
{
300316
sb.Append('/');
301317
}
302318

303-
sb.Append(actionName);
319+
sb.Append(actionName ?? "{action}");
304320
}
305321
else
306322
{

‎tracer/test/Datadog.Trace.Tests/DiagnosticListeners/AspNetCoreResourceNameHelperTests.cs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,12 +46,18 @@ public class AspNetCoreResourceNameHelperTests
4646
{ "{controller}/{action}/{id}", "/home/index/{id}", true },
4747
{ "{controller}/{action}", "/home/index", false },
4848
{ "{controller}/{action}", "/home/index", true },
49+
{ "{area:exists}/{controller}/{action}", "/{area}/home/index", false },
50+
{ "{area:exists}/{controller}/{action}", "/{area}/home/index", true },
4951
{ "prefix/{controller}/{action}", "/prefix/home/index", false },
5052
{ "prefix/{controller}/{action}", "/prefix/home/index", true },
5153
{ "prefix-{controller}/{action}-suffix", "/prefix-home/index-suffix", false },
5254
{ "prefix-{controller}/{action}-suffix", "/prefix-home/index-suffix", true },
5355
{ "prefix-{controller}-{action}-{nonid}-{id}-{FormValue}-suffix", "/prefix-home-index-{nonid}-{id}-{formvalue}-suffix", false },
5456
{ "prefix-{controller}-{action}-{nonid}-{id}-{FormValue}-suffix", "/prefix-home-index-oops-{id}-view-suffix", true },
57+
{ "prefix-{controller}-{action}-{Area}-{id}-{FormValue}-suffix", "/prefix-home-index-{area}-{id}-{formvalue}-suffix", false },
58+
{ "prefix-{controller}-{action}-{Area}-{id}-{FormValue}-suffix", "/prefix-home-index-{area}-{id}-view-suffix", true },
59+
{ "prefix-{controller}-{action}-{id}-{FormValue}/{Area?}", "/prefix-home-index-{id}-{formvalue}", false },
60+
{ "prefix-{controller}-{action}-{id}-{FormValue}/{Area?}", "/prefix-home-index-{id}-view", true },
5561
{ "standalone/prefix-{controller}-{action}-{nonid}-{id}-{FormValue}-suffix/standalone", "/standalone/prefix-home-index-{nonid}-{id}-{formvalue}-suffix/standalone", false },
5662
{ "standalone/prefix-{controller}-{action}-{nonid}-{id}-{FormValue}-suffix/standalone", "/standalone/prefix-home-index-oops-{id}-view-suffix/standalone", true },
5763
{ "{controller}/{action}/{nonid}", "/home/index/{nonid}", false },

0 commit comments

Comments
 (0)