Tools - Make Entities and Repositories nullable (#3313)

* support nullability in tools' entities and repositories

* enables C# nullability checks in these files
* includes documentation for affected files

* refine documentation per code review

* improve comments on SendFileData structure

* fix ReferenceEvent.MaxAccessCount documentation

* add value notation to SendFileData.FileName

src/Core/Tools/Entities/IReferenceable.cs

@@ -1,8 +1,29 @@
-namespace Bit.Core.Tools.Entities;
+#nullable enable
+using Bit.Core.Tools.Models.Business;
 
+namespace Bit.Core.Tools.Entities;
+
+/// <summary>
+/// An entity that can be referenced by a <see cref="ReferenceEvent"/>.
+/// </summary>
 public interface IReferenceable
 {
+    /// <summary>
+    /// Identifies the entity that generated the event.
+    /// </summary>
     Guid Id { get; set; }
-    string ReferenceData { get; set; }
+
+    /// <summary>
+    /// Contextual information included in the event.
+    /// </summary>
+    /// <remarks>
+    /// Do not store secrets in this field.
+    /// </remarks>
+    string? ReferenceData { get; set; }
+
+    /// <summary>
+    /// Returns <see langword="true" /> when the entity is a user.
+    /// Otherwise returns <see langword="false" />.
+    /// </summary>
     bool IsUser();
 }

src/Core/Tools/Entities/Send.cs

@@ -1,29 +1,125 @@
-using System.ComponentModel.DataAnnotations;
+#nullable enable
+
+using System.ComponentModel.DataAnnotations;
 using Bit.Core.Entities;
 using Bit.Core.Tools.Enums;
 using Bit.Core.Utilities;
 
 namespace Bit.Core.Tools.Entities;
 
+/// <summary>
+/// An end-to-end encrypted secret accessible to arbitrary
+/// entities through a fixed URI.
+/// </summary>
 public class Send : ITableObject<Guid>
 {
+    /// <summary>
+    /// Uniquely identifies this send.
+    /// </summary>
     public Guid Id { get; set; }
+
+    /// <summary>
+    /// Identifies the user that created this send.
+    /// </summary>
     public Guid? UserId { get; set; }
+
+    /// <summary>
+    /// Identifies the organization that created this send.
+    /// </summary>
+    /// <remarks>
+    /// Not presently in-use by client applications.
+    /// </remarks>
     public Guid? OrganizationId { get; set; }
+
+    /// <summary>
+    /// Describes the data being sent. This field determines how
+    /// the <see cref="Data"/> field is interpreted.
+    /// </summary>
     public SendType Type { get; set; }
-    public string Data { get; set; }
-    public string Key { get; set; }
+
+    /// <summary>
+    /// Stores data containing or pointing to the transmitted secret. JSON.
+    /// </summary>
+    /// <note>
+    /// Must be nullable due to several database column configuration.
+    /// The application and all other databases assume this is not nullable.
+    /// Tech debt ticket: PM-4128
+    /// </note>
+    public string? Data { get; set; }
+
+    /// <summary>
+    /// Stores the data's encryption key. Encrypted.
+    /// </summary>
+    /// <note>
+    /// Must be nullable due to MySql database column configuration.
+    /// The application and all other databases assume this is not nullable.
+    /// Tech debt ticket: PM-4128
+    /// </note>
+    public string? Key { get; set; }
+
+    /// <summary>
+    /// Password provided by the user. Protected with pbkdf2.
+    /// </summary>
     [MaxLength(300)]
-    public string Password { get; set; }
+    public string? Password { get; set; }
+
+    /// <summary>
+    /// The send becomes unavailable to API callers when
+    /// <see cref="AccessCount"/>  &gt;= <see cref="MaxAccessCount"/>.
+    /// </summary>
     public int? MaxAccessCount { get; set; }
+
+    /// <summary>
+    /// Number of times the content was accessed.
+    /// </summary>
+    /// <remarks>
+    /// This value is owned by the server. Clients cannot alter it.
+    /// </remarks>
     public int AccessCount { get; set; }
+
+    /// <summary>
+    /// The date this send was created.
+    /// </summary>
     public DateTime CreationDate { get; internal set; } = DateTime.UtcNow;
+
+    /// <summary>
+    /// The date this send was last modified.
+    /// </summary>
     public DateTime RevisionDate { get; internal set; } = DateTime.UtcNow;
+
+    /// <summary>
+    /// The date this send becomes unavailable to API callers.
+    /// </summary>
     public DateTime? ExpirationDate { get; set; }
+
+    /// <summary>
+    /// The date this send will be unconditionally deleted.
+    /// </summary>
+    /// <remarks>
+    /// This is set by server-side when the user doesn't specify a deletion date.
+    /// </remarks>
     public DateTime DeletionDate { get; set; }
+
+    /// <summary>
+    /// When this is true the send is not available to API callers,
+    /// unless they're the creator.
+    /// </summary>
     public bool Disabled { get; set; }
+
+    /// <summary>
+    /// Whether the creator's email address should be shown to the recipient.
+    /// </summary>
+    /// <value>
+    /// <see langword="false"/> indicates the email may be shown.
+    /// <see langword="true"/> indicates the email should be hidden.
+    /// <see langword="null"/> indicates the client doesn't set the field and
+    /// the email should be hidden.
+    /// </value>
     public bool? HideEmail { get; set; }
 
+    /// <summary>
+    /// Generates the send's <see cref="Id" />
+    /// </summary>
     public void SetNewId()
     {
         Id = CoreHelpers.GenerateComb();

src/Core/Tools/Models/Business/ReferenceEvent.cs

@@ -1,4 +1,6 @@
-using System.Text.Json.Serialization;
+#nullable enable
+
+using System.Text.Json.Serialization;
 using Bit.Core.Context;
 using Bit.Core.Enums;
 using Bit.Core.Tools.Entities;
@@ -6,10 +8,23 @@ using Bit.Core.Tools.Enums;
 
 namespace Bit.Core.Tools.Models.Business;
 
+/// <summary>
+/// Product support monitoring.
+/// </summary>
+/// <remarks>
+/// Do not store secrets in this type.
+/// </remarks>
 public class ReferenceEvent
 {
+    /// <summary>
+    /// Instantiates a <see cref="ReferenceEvent"/>.
+    /// </summary>
     public ReferenceEvent() { }
 
+    /// <inheritdoc cref="ReferenceEvent()" />
+    /// <param name="type">Monitored event type.</param>
+    /// <param name="source">Entity that created the event.</param>
+    /// <param name="currentContext">The conditions in which the event occurred.</param>
     public ReferenceEvent(ReferenceEventType type, IReferenceable source, ICurrentContext currentContext)
     {
         Type = type;
@@ -26,48 +41,197 @@ public class ReferenceEvent
         }
     }
 
+    /// <summary>
+    /// Monitored event type.
+    /// </summary>
     [JsonConverter(typeof(JsonStringEnumConverter))]
     public ReferenceEventType Type { get; set; }
 
+    /// <summary>
+    /// The kind of entity that created the event.
+    /// </summary>
     [JsonConverter(typeof(JsonStringEnumConverter))]
     public ReferenceEventSource Source { get; set; }
 
+    /// <inheritdoc cref="IReferenceable.Id"/>
     public Guid Id { get; set; }
 
-    public string ReferenceData { get; set; }
+    /// <inheritdoc cref="IReferenceable.ReferenceData"/>
+    public string? ReferenceData { get; set; }
 
+    /// <summary>
+    /// Moment the event occurred.
+    /// </summary>
     public DateTime EventDate { get; set; } = DateTime.UtcNow;
 
+    /// <summary>
+    /// Number of users sent invitations by an organization.
+    /// </summary>
+    /// <value>
+    /// Should contain a value only on <see cref="ReferenceEventType.InvitedUsers"/> events.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
     public int? Users { get; set; }
 
+    /// <summary>
+    /// Whether or not a subscription was canceled immediately or at the end of the billing period.
+    /// </summary>
+    /// <value>
+    /// <see langword="true"/> when a cancellation occurs immediately.
+    /// <see langword="false"/> when a cancellation occurs at the end of a customer's billing period.
+    /// Should contain a value only on <see cref="ReferenceEventType.CancelSubscription"/> events.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
     public bool? EndOfPeriod { get; set; }
 
-    public string PlanName { get; set; }
+    /// <summary>
+    /// Branded name of the subscription.
+    /// </summary>
+    /// <value>
+    /// Should contain a value only for subscription management events.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
+    public string? PlanName { get; set; }
 
+    /// <summary>
+    /// Identifies a subscription.
+    /// </summary>
+    /// <value>
+    /// Should contain a value only for subscription management events.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
     public PlanType? PlanType { get; set; }
 
-    public string OldPlanName { get; set; }
+    /// <summary>
+    /// The branded name of the prior plan.
+    /// </summary>
+    /// <value>
+    /// Should contain a value only on <see cref="ReferenceEventType.UpgradePlan"/> events
+    /// initiated by organizations.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
+    public string? OldPlanName { get; set; }
 
+    /// <summary>
+    /// Identifies the prior plan
+    /// </summary>
+    /// <value>
+    /// Should contain a value only on <see cref="ReferenceEventType.UpgradePlan"/> events
+    /// initiated by organizations.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
     public PlanType? OldPlanType { get; set; }
 
+    /// <summary>
+    /// Seat count when a billable action occurs. When adjusting seats, contains
+    /// the new seat count.
+    /// </summary>
+    /// <value>
+    /// Should contain a value only on <see cref="ReferenceEventType.Rebilled"/>,
+    /// <see cref="ReferenceEventType.AdjustSeats"/>, <see cref="ReferenceEventType.UpgradePlan"/>,
+    /// and <see cref="ReferenceEventType.Signup"/> events initiated by organizations.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
     public int? Seats { get; set; }
+
+    /// <summary>
+    /// Seat count when a seat adjustment occurs.
+    /// </summary>
+    /// <value>
+    /// Should contain a value only on <see cref="ReferenceEventType.AdjustSeats"/>
+    /// events initiated by organizations.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
     public int? PreviousSeats { get; set; }
 
+    /// <summary>
+    /// Qty in GB of storage. When adjusting storage, contains the adjusted
+    /// storage qty. Otherwise contains the total storage quantity.
+    /// </summary>
+    /// <value>
+    /// Should contain a value only on <see cref="ReferenceEventType.Rebilled"/>,
+    /// <see cref="ReferenceEventType.AdjustStorage"/>, <see cref="ReferenceEventType.UpgradePlan"/>,
+    /// and <see cref="ReferenceEventType.Signup"/> events.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
     public short? Storage { get; set; }
 
+    /// <summary>
+    /// The type of send created or accessed.
+    /// </summary>
+    /// <value>
+    /// Should contain a value only on <see cref="ReferenceEventType.SendAccessed"/>
+    /// and <see cref="ReferenceEventType.SendCreated"/> events.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
     [JsonConverter(typeof(JsonStringEnumConverter))]
     public SendType? SendType { get; set; }
 
+    /// <summary>
+    /// Whether the send has private notes.
+    /// </summary>
+    /// <value>
+    /// <see langword="true"/> when the send has private notes, otherwise <see langword="false"/>.
+    /// Should contain a value only on <see cref="ReferenceEventType.SendAccessed"/>
+    /// and <see cref="ReferenceEventType.SendCreated"/> events.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
     public bool? SendHasNotes { get; set; }
 
+    /// <summary>
+    /// The send expires after its access count exceeds this value.
+    /// </summary>
+    /// <value>
+    /// This field only contains a value when the send has a max access count
+    /// and <see cref="Type"/> is <see cref="ReferenceEventType.SendAccessed"/>
+    /// or <see cref="ReferenceEventType.SendCreated"/> events.
+    /// Otherwise, the value should be <see langword="null"/>.
+    /// </value>
     public int? MaxAccessCount { get; set; }
 
+    /// <summary>
+    /// Whether the created send has a password.
+    /// </summary>
+    /// <value>
+    /// Should contain a value only on <see cref="ReferenceEventType.SendAccessed"/>
+    /// and <see cref="ReferenceEventType.SendCreated"/> events.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
     public bool? HasPassword { get; set; }
 
-    public string EventRaisedByUser { get; set; }
+    /// <summary>
+    /// The administrator that performed the action.
+    /// </summary>
+    /// <value>
+    /// Should contain a value only on <see cref="ReferenceEventType.OrganizationCreatedByAdmin"/>
+    /// and <see cref="ReferenceEventType.OrganizationEditedByAdmin"/> events.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
+    public string? EventRaisedByUser { get; set; }
 
+    /// <summary>
+    /// Whether or not an organization's trial period was started by a sales person.
+    /// </summary>
+    /// <value>
+    /// Should contain a value only on <see cref="ReferenceEventType.OrganizationCreatedByAdmin"/>
+    /// and <see cref="ReferenceEventType.OrganizationEditedByAdmin"/> events.
+    /// Otherwise the value should be <see langword="null"/>.
+    /// </value>
     public bool? SalesAssistedTrialStarted { get; set; }
 
-    public string ClientId { get; set; }
-    public Version ClientVersion { get; set; }
+    /// <summary>
+    /// The installation id of the application that originated the event.
+    /// </summary>
+    /// <value>
+    /// <see langword="null"/> when the event was not originated by an application.
+    /// </value>
+    public string? ClientId { get; set; }
+
+    /// <summary>
+    /// The version of the client application that originated the event.
+    /// </summary>
+    /// <value>
+    /// <see langword="null"/> when the event was not originated by an application.
+    /// </value>
+    public Version? ClientVersion { get; set; }
 }

src/Core/Tools/Models/Data/SendData.cs

@@ -1,15 +1,33 @@
-namespace Bit.Core.Tools.Models.Data;
+#nullable enable
 
+namespace Bit.Core.Tools.Models.Data;
+
+/// <summary>
+/// Shared data for a send
+/// </summary>
 public abstract class SendData
 {
+    /// <summary>
+    /// Instantiates a <see cref="SendData"/>.
+    /// </summary>
     public SendData() { }
 
-    public SendData(string name, string notes)
+    /// <inheritdoc cref="SendData()" />
+    /// <param name="name">User-provided name of the send.</param>
+    /// <param name="notes">User-provided private notes of the send.</param>
+    public SendData(string name, string? notes)
     {
         Name = name;
         Notes = notes;
     }
 
-    public string Name { get; set; }
-    public string Notes { get; set; }
+    /// <summary>
+    /// User-provided name of the send.
+    /// </summary>
+    public string Name { get; set; } = string.Empty;
+
+    /// <summary>
+    /// User-provided private notes of the send.
+    /// </summary>
+    public string? Notes { get; set; } = null;
 }

src/Core/Tools/Models/Data/SendFileData.cs

@@ -1,22 +1,64 @@
-using System.Text.Json.Serialization;
+#nullable enable
+
+using System.Diagnostics.CodeAnalysis;
+using System.Text.Json.Serialization;
+using static System.Text.Json.Serialization.JsonNumberHandling;
 
 namespace Bit.Core.Tools.Models.Data;
 
+/// <summary>
+/// A file secret being sent.
+/// </summary>
 public class SendFileData : SendData
 {
+    /// <summary>
+    /// Instantiates a <see cref="SendFileData"/>.
+    /// </summary>
     public SendFileData() { }
 
-    public SendFileData(string name, string notes, string fileName)
+    /// <inheritdoc cref="SendFileData()"/>
+    /// <param name="name">Attached file name.</param>
+    /// <param name="notes">User-provided private notes of the send.</param>
+    /// <param name="fileName">Attached file name.</param>
+    public SendFileData(string name, string? notes, string fileName)
         : base(name, notes)
     {
         FileName = fileName;
     }
 
-    // We serialize Size as a string since JSON (or Javascript) doesn't support full precision for long numbers
-    [JsonNumberHandling(JsonNumberHandling.WriteAsString | JsonNumberHandling.AllowReadingFromString)]
+    /// <summary>
+    /// Size of the attached file in bytes.
+    /// </summary>
+    /// <remarks>
+    /// Serialized as a string since JSON (or Javascript)  doesn't support
+    /// full precision for long numbers
+    /// </remarks>
+    [JsonNumberHandling(WriteAsString | AllowReadingFromString)]
     public long Size { get; set; }
 
-    public string Id { get; set; }
-    public string FileName { get; set; }
+    /// <summary>
+    /// Uniquely identifies an uploaded file.
+    /// </summary>
+    /// <value>
+    /// Should contain <see langword="null" /> only when a file
+    /// upload is pending. Should never contain null once the
+    /// file upload completes.
+    /// </value>
+    [DisallowNull]
+    public string? Id { get; set; }
+
+    /// <summary>
+    /// Attached file name.
+    /// </summary>
+    /// <value>
+    /// Should contain a non-empty string once the file upload completes.
+    /// </value>
+    public string FileName { get; set; } = string.Empty;
+
+    /// <summary>
+    /// When true the uploaded file's length was confirmed within
+    /// the expected tolerance and below the maximum supported
+    /// file size.
+    /// </summary>
     public bool Validated { get; set; } = true;
 }

src/Core/Tools/Models/Data/SendTextData.cs

@@ -1,16 +1,42 @@
-namespace Bit.Core.Tools.Models.Data;
+#nullable enable
 
+namespace Bit.Core.Tools.Models.Data;
+
+/// <summary>
+/// A text secret being sent.
+/// </summary>
 public class SendTextData : SendData
 {
+    /// <summary>
+    /// Instantiates a <see cref="SendTextData"/>.
+    /// </summary>
     public SendTextData() { }
 
-    public SendTextData(string name, string notes, string text, bool hidden)
+    /// <inheritdoc cref="SendTextData()"/>
+    /// <param name="name">Attached file name.</param>
+    /// <param name="notes">User-provided private notes of the send.</param>
+    /// <param name="text">The secret being sent.</param>
+    /// <param name="hidden">
+    /// Indicates whether the secret should be concealed when opening the send.
+    /// </param>
+    public SendTextData(string name, string? notes, string? text, bool hidden)
         : base(name, notes)
     {
         Text = text;
         Hidden = hidden;
     }
 
-    public string Text { get; set; }
+    /// <summary>
+    /// The secret being sent.
+    /// </summary>
+    public string? Text { get; set; }
+
+    /// <summary>
+    /// Indicates whether the secret should be concealed when opening the send.
+    /// </summary>
+    /// <value>
+    /// <see langword="true" /> when the secret should be concealed.
+    /// Otherwise <see langword="false" />.
+    /// </value>
     public bool Hidden { get; set; }
 }

src/Core/Tools/Repositories/ISendRepository.cs

@@ -1,10 +1,36 @@
-using Bit.Core.Repositories;
+#nullable enable
+
+using Bit.Core.Repositories;
 using Bit.Core.Tools.Entities;
 
 namespace Bit.Core.Tools.Repositories;
 
+/// <summary>
+/// Service for saving and loading <see cref="Send"/>s in persistent storage.
+/// </summary>
 public interface ISendRepository : IRepository<Send, Guid>
 {
+    /// <summary>
+    /// Loads all <see cref="Send"/>s created by a user.
+    /// </summary>
+    /// <param name="userId">
+    /// Identifies the user.
+    /// </param>
+    /// <returns>
+    /// A task that completes once the <see cref="Send"/>s have been loaded.
+    /// The task's result contains the loaded <see cref="Send"/>s.
+    /// </returns>
     Task<ICollection<Send>> GetManyByUserIdAsync(Guid userId);
+
+    /// <summary>
+    /// Loads <see cref="Send"/>s scheduled for deletion.
+    /// </summary>
+    /// <param name="deletionDateBefore">
+    /// Load sends whose <see cref="Send.DeletionDate" /> is &lt; this date.
+    /// </param>
+    /// <returns>
+    /// A task that completes once the <see cref="Send"/>s have been loaded.
+    /// The task's result contains the loaded <see cref="Send"/>s.
+    /// </returns>
     Task<ICollection<Send>> GetManyByDeletionDateAsync(DateTime deletionDateBefore);
 }

src/Infrastructure.Dapper/Tools/Repositories/SendRepository.cs

@@ -1,4 +1,6 @@
-using System.Data;
+#nullable enable
+
+using System.Data;
 using Bit.Core.Settings;
 using Bit.Core.Tools.Entities;
 using Bit.Core.Tools.Repositories;
@@ -8,6 +10,7 @@ using Microsoft.Data.SqlClient;
 
 namespace Bit.Infrastructure.Dapper.Tools.Repositories;
 
+/// <inheritdoc cref="ISendRepository" />
 public class SendRepository : Repository<Send, Guid>, ISendRepository
 {
     public SendRepository(GlobalSettings globalSettings)
@@ -18,6 +21,7 @@ public class SendRepository : Repository<Send, Guid>, ISendRepository
         : base(connectionString, readOnlyConnectionString)
     { }
 
+    /// <inheritdoc />
     public async Task<ICollection<Send>> GetManyByUserIdAsync(Guid userId)
     {
         using (var connection = new SqlConnection(ConnectionString))
@@ -31,6 +35,7 @@ public class SendRepository : Repository<Send, Guid>, ISendRepository
         }
     }
 
+    /// <inheritdoc />
     public async Task<ICollection<Send>> GetManyByDeletionDateAsync(DateTime deletionDateBefore)
     {
         using (var connection = new SqlConnection(ConnectionString))

src/Infrastructure.EntityFramework/Tools/Repositories/SendRepository.cs

@@ -1,4 +1,6 @@
-using AutoMapper;
+#nullable enable
+
+using AutoMapper;
 using Bit.Core.Tools.Repositories;
 using Bit.Infrastructure.EntityFramework.Models;
 using Bit.Infrastructure.EntityFramework.Repositories;
@@ -7,12 +9,28 @@ using Microsoft.Extensions.DependencyInjection;
 
 namespace Bit.Infrastructure.EntityFramework.Tools.Repositories;
 
+/// <inheritdoc cref="ISendRepository"/>
 public class SendRepository : Repository<Core.Tools.Entities.Send, Send, Guid>, ISendRepository
 {
+    /// <summary>
+    /// Initializes the <see cref="SendRepository"/>
+    /// </summary>
+    /// <param name="serviceScopeFactory">An IoC service locator.</param>
+    /// <param name="mapper">An automapper service.</param>
     public SendRepository(IServiceScopeFactory serviceScopeFactory, IMapper mapper)
         : base(serviceScopeFactory, mapper, (DatabaseContext context) => context.Sends)
     { }
 
+    /// <summary>
+    /// Saves a <see cref="Send"/> in the database.
+    /// </summary>
+    /// <param name="send">
+    /// The send being saved.
+    /// </param>
+    /// <returns>
+    /// A task that completes once the save is complete.
+    /// The task result contains the saved <see cref="Send"/>.
+    /// </returns>
     public override async Task<Core.Tools.Entities.Send> CreateAsync(Core.Tools.Entities.Send send)
     {
         send = await base.CreateAsync(send);
@@ -30,6 +48,7 @@ public class SendRepository : Repository<Core.Tools.Entities.Send, Send, Guid>,
         return send;
     }
 
+    /// <inheritdoc />
     public async Task<ICollection<Core.Tools.Entities.Send>> GetManyByDeletionDateAsync(DateTime deletionDateBefore)
     {
         using (var scope = ServiceScopeFactory.CreateScope())
@@ -40,6 +59,7 @@ public class SendRepository : Repository<Core.Tools.Entities.Send, Send, Guid>,
         }
     }
 
+    /// <inheritdoc />
     public async Task<ICollection<Core.Tools.Entities.Send>> GetManyByUserIdAsync(Guid userId)
     {
         using (var scope = ServiceScopeFactory.CreateScope())