Skip to content

Bug: SqlTopicRepository.Refresh() won't delete attributes #45

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
JeremyCaney opened this issue Mar 12, 2021 · 1 comment
Closed

Bug: SqlTopicRepository.Refresh() won't delete attributes #45

JeremyCaney opened this issue Mar 12, 2021 · 1 comment
Assignees
@JeremyCaney
Labels
Priority: 1 Severity 1: Minor Status 5: Complete Task is considered complete, and ready for deployment. Type: Bug Behavior that is inconsistent with documented or expected behavior.
Milestone
OnTopic 5.1.0

Comments

@JeremyCaney
Copy link
Member

Attributes can be null when returned from the database. This happens when a previous attribute was deleted; the new version will use a null value. When loading values as part of e.g. ITopicRepository.Refresh() or Rollback(), this should result in deleting any existing attribute values, if present. Currently, this does not happen.

Impact

If the null values are skipped, any previous values will be retained in memory. There may also be circumstances where this could result in those values being inadvertently persisted to the database—though, fortunately, this will generally be avoided by the fact that Save() doesn't include clean indexed attributes when calling the SaveTopic stored procedure.

Cause

The SetIndexedAttributes() extension method skips over null values. That was fine when this was only used to create new topics. With the OnTopic 5.0.0 support for passing a referenceTopic to Load(), however, this potentially prevents existing topics from being properly updated with new attributes. This is most notable with ITopicRepository.Refresh(), since the goal is explicitly to update existing topics with new values.
@JeremyCaney JeremyCaney added Type: Bug Behavior that is inconsistent with documented or expected behavior. Severity 1: Minor Priority: 1 Status 2: Scheduled Planned for an upcoming release. labels Mar 12, 2021
@JeremyCaney JeremyCaney added this to the OnTopic 5.1.0 milestone Mar 12, 2021
@JeremyCaney JeremyCaney self-assigned this Mar 12, 2021
@JeremyCaney
Copy link
Member Author

This is resolved by 47678b1. This necessitated also introducing a new GetNullableString() extension method to support the DbNull handling. This is covered by an existing unit test (a48c3e7).

@JeremyCaney JeremyCaney added Status 5: Complete Task is considered complete, and ready for deployment. and removed Status 2: Scheduled Planned for an upcoming release. labels Mar 12, 2021
@JeremyCaney JeremyCaney mentioned this issue Mar 12, 2021
Closed
JeremyCaney added a commit that referenced this issue Mar 13, 2021
@JeremyCaney
Merge branch 'maintenance/unit-test-coverage' into develop
58dec57 
Went through Azure DevOps' code coverage report, and used it to patch gaps in our unit tests. There remain a lot of gaps in the `OnTopic.Data.Sql` and the `OnTopic.AspNetCore.Mvc` libraries, which will be better served by integration tests. Outside of those two libraries, we're now above 99% code coverage on the main libraries.

As part of this expanded coverage, I identified and resolved a number of bugs that hadn't yet been identified, particularly around the `ITopicRepository` implementations. These include:
- `SqlTopicRepository.Load()` won't delete attributes (#45) or topic references (#46)
- `SqlTopicRepository.Load()` treats `Topic.References` as `!IsFullyLoaded` if there are any deleted references (#46)
- `TopicRepository.Load(topic, version)` validates `version` against `DateTime.Now` not `DateTime.UtcNow` (#56)
- `TopicRepository.Delete()` throws exception if the expected `ReturnCode` is missing (#48)
- `TopicRepositoryDecorator.Delete()` didn't set default value for the `isRecursive` parameter (#49)
- `TopicRepository.TopicRenamed` event not being raised on `Save()` (#50)
- `Topic.IncomingRelationships` aren't being properly cleared by calls to `TopicReferenceCollection.Clear()` (#51) and `SetValue()` (#52)
- `Topic.IsDirty()` is being set to `true` if `Topic.Parent` is set during construction, even if it's an existing entity (#53)
- `ReadOnlyKeyedTopicCollection` constructor requires nullable, optional `innerCollection` parameter (#54)
- `ContentTypeDescriptor.IsTypeOf()` always returns `true` and thus is non-functional (#55)
- `TrackedRecord<T>` and derivative constructors don't accept a null `value`, even though the corresponding `Value` property does (#59)

In addition, it identified two bugs which are scheduled for OnTopic 5.1.0, but which haven't yet been resolved:
- `TopicRepository.GetAttributes()` returns mismatched attributes for both `isDirty` and `!isDirty` (#44)
- `SqlTopicRepository.TopicLoaded` event is not raised with the `version` parameter where appropriate (#58)
@JeremyCaney JeremyCaney changed the title Bug: SqlTopicRepository.Refresh() won't delete attributes Bug: SqlTopicRepository.Refresh() won't delete attributes Mar 29, 2021
@JeremyCaney JeremyCaney mentioned this issue Apr 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@JeremyCaney JeremyCaney

Labels
Priority: 1 Severity 1: Minor Status 5: Complete Task is considered complete, and ready for deployment. Type: Bug Behavior that is inconsistent with documented or expected behavior.
Projects
None yet
Milestone
OnTopic 5.1.0
Development

No branches or pull requests
1 participant
@JeremyCaney