Skip to content

Commit 2dbd5db

Browse files
RichardIrons-neo4jRichardIrons-neo4j
authored
Fix for Stefan's category/classification issue in WithNotifications (#834)
* Fix for Stefan's category/classification issue in WithNotifications, some tidying, moved GQL Notifications out of preview * Add test for WithNotifications to set multiple categories and classifications * Refactor NotificationsConfig to handle null disabled categories and classifications * Update tests to assert DisabledCategories is empty instead of null
1 parent 6659e14 commit 2dbd5db

File tree

12 files changed

+157
-139
lines changed

12 files changed

+157
-139
lines changed

‎Neo4j.Driver/Neo4j.Driver.Tests/ConfigTests.cs‎

Lines changed: 82 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,7 @@ public void WithNotifications_ShouldSetCategoryWithClassification(
256256
{
257257
var configBuilder = new ConfigBuilder(new Config());
258258

259-
configBuilder.WithNotifications(null, [classification]);
259+
configBuilder.WithNotifications(null, disabledClassifications: [classification]);
260260

261261
var config = configBuilder.Build()
262262
.NotificationsConfig.Should()
@@ -294,10 +294,12 @@ public void WithNotifications_ShouldSetCategory(
294294
var config = configBuilder.Build()
295295
.NotificationsConfig.Should()
296296
.BeOfType<NotificationsConfig>();
297+
297298
config
298299
.Which
299300
.DisabledCategories.Should()
300301
.BeEquivalentTo([outCat]);
302+
301303
config
302304
.Which
303305
.MinimumSeverity.Should()
@@ -331,7 +333,9 @@ public void WithNotifications_ShouldSetMultipleClassifications()
331333
{
332334
var configBuilder = new ConfigBuilder(new Config());
333335

334-
configBuilder.WithNotifications(null, [Classification.Deprecation, Classification.Hint]);
336+
configBuilder.WithNotifications(
337+
null,
338+
disabledClassifications: [Classification.Deprecation, Classification.Hint]);
335339

336340
var config = configBuilder.Build()
337341
.NotificationsConfig.Should()
@@ -362,7 +366,7 @@ public void WithNotifications_ShouldSetSeverity()
362366
config
363367
.Which
364368
.DisabledCategories.Should()
365-
.BeEquivalentTo([]);
369+
.BeEmpty();
366370

367371
config
368372
.Which
@@ -375,7 +379,79 @@ public void WithNotifications_ShouldSetSeverityWhenUsingClassification()
375379
{
376380
var configBuilder = new ConfigBuilder(new Config());
377381

378-
configBuilder.WithNotifications(Severity.Warning, Array.Empty<Classification>());
382+
configBuilder.WithNotifications(Severity.Warning, disabledClassifications: Array.Empty<Classification>());
383+
384+
var config = configBuilder.Build()
385+
.NotificationsConfig.Should()
386+
.BeOfType<NotificationsConfig>();
387+
388+
config
389+
.Which
390+
.DisabledCategories.Should()
391+
.BeEmpty();
392+
393+
config
394+
.Which
395+
.MinimumSeverity.Should()
396+
.Be(Severity.Warning);
397+
}
398+
399+
[Fact]
400+
public void WithNotifications_ShouldWorkWithSecondParameterNull()
401+
{
402+
var configBuilder = new ConfigBuilder(new Config());
403+
404+
// this line would fail to compile before the fix
405+
configBuilder.WithNotifications(Severity.Warning, null);
406+
407+
var config = configBuilder.Build()
408+
.NotificationsConfig.Should()
409+
.BeOfType<NotificationsConfig>();
410+
411+
config
412+
.Which
413+
.DisabledCategories.Should()
414+
.BeNull();
415+
416+
config
417+
.Which
418+
.MinimumSeverity.Should()
419+
.Be(Severity.Warning);
420+
}
421+
422+
[Fact]
423+
public void WithNotifications_ShouldSetMultipleCategoriesAndClassifications()
424+
{
425+
var configBuilder = new ConfigBuilder(new Config());
426+
427+
// specify both categories and classifications
428+
configBuilder.WithNotifications(
429+
Severity.Warning,
430+
[Category.Deprecation, Category.Hint],
431+
[Classification.Deprecation, Classification.Topology]);
432+
433+
var config = configBuilder.Build()
434+
.NotificationsConfig.Should()
435+
.BeOfType<NotificationsConfig>();
436+
437+
config
438+
.Which
439+
.DisabledCategories.Should()
440+
.BeEquivalentTo([Category.Deprecation, Category.Hint, Category.Topology]);
441+
442+
config
443+
.Which
444+
.MinimumSeverity.Should()
445+
.Be(Severity.Warning);
446+
}
447+
448+
[Fact]
449+
public void WithNotifications_ShouldHaveNullExclusions()
450+
{
451+
var configBuilder = new ConfigBuilder(new Config());
452+
453+
// this line would fail to compile before the fix
454+
configBuilder.WithNotifications(Severity.Warning, null);
379455

380456
var config = configBuilder.Build()
381457
.NotificationsConfig.Should()
@@ -384,14 +460,14 @@ public void WithNotifications_ShouldSetSeverityWhenUsingClassification()
384460
config
385461
.Which
386462
.DisabledCategories.Should()
387-
.BeEquivalentTo([]);
463+
.BeNull();
388464

389465
config
390466
.Which
391467
.MinimumSeverity.Should()
392468
.Be(Severity.Warning);
393469
}
394-
470+
395471
private class MockTlsNegotiator : ITlsNegotiator
396472
{
397473
/// <inheritdoc/>

‎Neo4j.Driver/Neo4j.Driver.Tests/SessionConfigBuilderTests.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ public void WithNotifications_ShouldSetCategoryWithClassification(
3838
{
3939
var configBuilder = new SessionConfigBuilder(new SessionConfig());
4040

41-
configBuilder.WithNotifications(null, [classification]);
41+
configBuilder.WithNotifications(null, disabledClassifications: [classification]);
4242

4343
var config = configBuilder.Build()
4444
.NotificationsConfig.Should()
@@ -115,7 +115,7 @@ public void WithNotifications_ShouldSetMultipleClassifications()
115115
{
116116
var configBuilder = new SessionConfigBuilder(new SessionConfig());
117117

118-
configBuilder.WithNotifications(null, [Classification.Deprecation, Classification.Hint]);
118+
configBuilder.WithNotifications(null, disabledClassifications: [Classification.Deprecation, Classification.Hint]);
119119

120120
var config = configBuilder.Build()
121121
.NotificationsConfig.Should()
@@ -146,7 +146,7 @@ public void WithNotifications_ShouldSetSeverity()
146146
config
147147
.Which
148148
.DisabledCategories.Should()
149-
.BeEquivalentTo([]);
149+
.BeEmpty();
150150

151151
config
152152
.Which
@@ -159,7 +159,7 @@ public void WithNotifications_ShouldSetSeverityWhenUsingClassification()
159159
{
160160
var configBuilder = new SessionConfigBuilder(new SessionConfig());
161161

162-
configBuilder.WithNotifications(Severity.Warning, Array.Empty<Classification>());
162+
configBuilder.WithNotifications(Severity.Warning, disabledClassifications: Array.Empty<Classification>());
163163

164164
var config = configBuilder.Build()
165165
.NotificationsConfig.Should()
@@ -168,7 +168,7 @@ public void WithNotifications_ShouldSetSeverityWhenUsingClassification()
168168
config
169169
.Which
170170
.DisabledCategories.Should()
171-
.BeEquivalentTo([]);
171+
.BeEmpty();
172172

173173
config
174174
.Which

‎Neo4j.Driver/Neo4j.Driver/Preview/GqlErrors/IGqlErrorPreview.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ public interface IGqlErrorPreview
4949
public Dictionary<string, object> GqlDiagnosticRecord { get; }
5050
}
5151

52-
public static class Neo4jExceptionExtensions
52+
internal static class Neo4jExceptionExtensions
5353
{
5454
public static IGqlErrorPreview GetGqlErrorPreview(this Neo4jException exception)
5555
{

‎Neo4j.Driver/Neo4j.Driver/Public/ConfigBuilder.cs‎

Lines changed: 30 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,18 @@ public ConfigBuilder WithCertificateTrustRule(
387387
return WithCertificateTrustRule(certificateTrustRule, certs);
388388
}
389389

390+
/// <summary>Disable all notifications for the lifetime of the driver.</summary>
391+
/// <remarks>Cannot be used with: <see cref="WithNotifications(Severity?, Category[], Classification[])"/>.</remarks>
392+
/// <returns>A <see cref="ConfigBuilder"/> instance for further configuration options.</returns>
393+
/// <seealso cref="WithNotifications(Severity?, Category[], Classification[])"/>
394+
/// <seealso cref="SessionConfigBuilder.WithNotifications(Severity?, Category[], Classification[])"/>
395+
/// <seealso cref="SessionConfigBuilder.WithNotificationsDisabled"/>
396+
public ConfigBuilder WithNotificationsDisabled()
397+
{
398+
_config.NotificationsConfig = new NotificationsDisabledConfig();
399+
return this;
400+
}
401+
390402
/// <summary>
391403
/// Override configuration for which <see cref="INotification"/>s should be emitted for the lifetime of the
392404
/// driver. <br/> Unspecified configuration will be provided by configuration in the server. <br/> Disabling categories or
@@ -402,82 +414,38 @@ public ConfigBuilder WithCertificateTrustRule(
402414
/// an empty collection, all categories are enabled.<br/> By leaving null, the value will inherit configuration from the
403415
/// server.
404416
/// </param>
405-
/// <exception cref="ArgumentException">Thrown when both parameters are null.</exception>
417+
/// <param name="disabledClassifications">
418+
/// Optional parameter to override the classification of notifications emitted. <br/> By passing
419+
/// an empty collection, all classifications are enabled.<br/> By leaving null, the value will inherit configuration from the
420+
/// server.
421+
/// </param>
422+
/// <exception cref="ArgumentException">Thrown when all parameters are null.</exception>
406423
/// <returns>A <see cref="ConfigBuilder"/> instance for further configuration options.</returns>
407424
/// <seealso cref="WithNotificationsDisabled"/>
408-
/// <seealso cref="SessionConfigBuilder.WithNotifications(Severity?, Category[])"/>"/>
425+
/// <seealso cref="SessionConfigBuilder.WithNotifications(Severity?, Category[], Classification[])"/>"/>
409426
/// <seealso cref="SessionConfigBuilder.WithNotificationsDisabled"/>
410427
/// <returns>A <see cref="ConfigBuilder"/> instance for further configuration options.</returns>
411428
public ConfigBuilder WithNotifications(
412429
Severity? minimumSeverity,
413-
Category[] disabledCategories)
430+
Category[] disabledCategories = null,
431+
Classification[] disabledClassifications = null)
414432
{
415-
if (minimumSeverity == null && disabledCategories == null)
433+
if (minimumSeverity == null && disabledCategories == null && disabledClassifications == null)
416434
{
417435
throw new ArgumentException(
418-
$"Both {nameof(minimumSeverity)} and {nameof(disabledCategories)} are both null, at least one must be non-null.");
436+
$"All {nameof(minimumSeverity)}, {nameof(disabledCategories)} and {nameof(disabledClassifications)} " +
437+
"are null, at least one must be non-null.");
419438
}
420439

421-
_config.NotificationsConfig = new NotificationsConfig(minimumSeverity, disabledCategories);
422-
return this;
423-
}
424-
425-
/// <summary>Disable all notifications for the lifetime of the driver.</summary>
426-
/// <remarks>Cannot be used with: <see cref="WithNotifications(Severity?, Category[])"/>.</remarks>
427-
/// <returns>A <see cref="ConfigBuilder"/> instance for further configuration options.</returns>
428-
/// <seealso cref="WithNotifications(Severity?, Category[])"/>
429-
/// <seealso cref="SessionConfigBuilder.WithNotifications(Severity?, Category[])"/>
430-
/// <seealso cref="SessionConfigBuilder.WithNotificationsDisabled"/>
431-
public ConfigBuilder WithNotificationsDisabled()
432-
{
433-
_config.NotificationsConfig = new NotificationsDisabledConfig();
434-
return this;
435-
}
436-
437-
438-
/// <summary>
439-
/// This is a preview API, This API may change between minor revisions.<br/>
440-
///
441-
/// Override configuration for which <see cref="IGqlStatusObject"/> and <see cref="INotification"/> should be emitted
442-
/// for the lifetime of the session. <br/> Unspecified configuration will be provided by configuration specified in
443-
/// the server or the driver's <see cref="ConfigBuilder.WithNotifications(Severity?, Classification[])"/> or
444-
/// <see cref="ConfigBuilder.WithNotifications(Severity?, Category[])"/>. <br/> If the driver has disabled
445-
/// notifications with <see cref="ConfigBuilder.WithNotificationsDisabled"/>, the unspecified values will be
446-
/// provided by the server. <br/> Disabling categories, classifications or severities allows the server to skip
447-
/// analysis for those which can speed up query execution.
448-
/// </summary>
449-
/// <remarks>Cannot be used with: <see cref="WithNotificationsDisabled"/> or
450-
/// <see cref="WithNotifications(Severity?, Category[])"/>.</remarks>
451-
/// <param name="minimumSeverity">
452-
/// Optional parameter to override the minimum severity of notifications emitted. <br/> By
453-
/// leaving null, the value will inherit configuration of <see cref="Config.NotificationsConfig"/> or the server.
454-
/// </param>
455-
/// <param name="disabledClassifications">
456-
/// Optional parameter to override the category of notifications and classifications of GQL Statuses emitted. <br/>
457-
/// By passing an empty collection, all categories are enabled.<br/> By leaving null, the value will inherit
458-
/// configuration from <see cref="Config.NotificationsConfig"/> or the server.
459-
/// </param>
460-
/// <exception cref="ArgumentException">Thrown when both parameters are null.</exception>
461-
/// <returns>A <see cref="SessionConfigBuilder"/> instance for further configuration options.</returns>
462-
/// <seealso cref="WithNotificationsDisabled"/>
463-
/// <seealso cref="WithNotifications(Severity?, Category[])"/>
464-
/// <seealso cref="ConfigBuilder.WithNotifications(Severity?, Category[])"/>
465-
/// <seealso cref="ConfigBuilder.WithNotifications(Severity?, Classification[])"/>
466-
/// <seealso cref="ConfigBuilder.WithNotificationsDisabled"/>
467-
/// <since>5.23.0</since>
468-
[Obsolete("This is a Preview API and may change between minor versions. Obsolete will be removed in a later revision.")]
469-
public ConfigBuilder WithNotifications(
470-
Severity? minimumSeverity,
471-
Classification[] disabledClassifications)
472-
{
473-
if (minimumSeverity == null && disabledClassifications == null)
440+
Category[] categoriesToDisable = null;
441+
if (disabledCategories != null || disabledClassifications != null)
474442
{
475-
throw new ArgumentException(
476-
$"Both {nameof(minimumSeverity)} and {nameof(disabledClassifications)} are both null, at least one must be non-null.");
443+
var classificationsAsCategories = disabledClassifications?.Select(x => (Category)(int)x) ?? [];
444+
categoriesToDisable = (disabledCategories ?? []).Concat(classificationsAsCategories).ToArray();
477445
}
478446

479-
_config.NotificationsConfig = new NotificationsConfig(minimumSeverity,
480-
disabledClassifications.Select(x => (Category)(int)x).ToArray());
447+
_config.NotificationsConfig = new NotificationsConfig(minimumSeverity, categoriesToDisable);
448+
481449
return this;
482450
}
483451

‎Neo4j.Driver/Neo4j.Driver/Public/Mapping/ExecutableQueryMappingExtensions.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ public static async Task<IReadOnlyList<T>> AsObjectsAsync<T>(
4747
/// </summary>
4848
/// <seealso cref="RecordObjectMapping.Map{T}"/>
4949
/// <param name="recordsTask">The task that will return the records.</param>
50+
/// <param name="blueprint">The blueprint to use for mapping.</param>
5051
/// <typeparam name="T">The type to map to.</typeparam>
5152
/// <returns>A task that will return the mapped objects.</returns>
5253
public static async Task<IReadOnlyList<T>> AsObjectsFromBlueprintAsync<T>(

‎Neo4j.Driver/Neo4j.Driver/Public/Mapping/MappingFailedException.cs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,19 @@ namespace Neo4j.Driver.Mapping;
2222
/// </summary>
2323
public class MappingFailedException : Neo4jException
2424
{
25+
/// <summary>
26+
/// Initializes a new instance of the <see cref="MappingFailedException"/> class.
27+
/// </summary>
28+
/// <param name="message">The message that describes the error.</param>
2529
public MappingFailedException(string message) : base(message)
2630
{
2731
}
2832

33+
/// <summary>
34+
/// Initializes a new instance of the <see cref="MappingFailedException"/> class.
35+
/// </summary>
36+
/// <param name="message">The message that describes the error.</param>
37+
/// <param name="innerException">The exception that is the cause of the current exception.</param>
2938
public MappingFailedException(string message, Exception innerException) : base(message, innerException)
3039
{
3140
}

0 commit comments

Comments
 (0)