Skip to content

Add ValueComparer for JSON DOM types using DeepEquals#3887

Draft
hostage2222 wants to merge 1 commit into
npgsql:mainfrom
hostage2222:json-dom-change-tracking
Draft

Add ValueComparer for JSON DOM types using DeepEquals#3887
hostage2222 wants to merge 1 commit into
npgsql:mainfrom
hostage2222:json-dom-change-tracking

Conversation

@hostage2222

Copy link
Copy Markdown

The change tracker currently uses shallow comparison when detecting changes to JSON DOM properties (JsonDocument and JsonElement). If the JSON content itself hasn't actually changed, but a new object instance was created, this results in unnecessary saves to the database.

This PR adds ValueComparers for JsonDocument and JsonElement based on DeepEquals.

Trade-offs:

  • For performance, the snapshot stores the original JsonDocument/JsonElement instance rather than a deep copy. If the original JsonDocument has since been disposed, comparing against the snapshot will throw.
  • Key order doesn't matter for jsonb, but does for json. DeepEquals follows jsonb's semantics either way.
  • JsonElement.GetHashCode() is inefficient, so JsonElementComparer always returns 0 as its hash.
  • JsonDocumentComparer's GetHashCode() is reference-based while Equals compares by content, breaking the Equals/GetHashCode contract — same trade-off as HstoreMutableComparer for Dictionary<string, string>.

As a very rough estimate of the potential performance gain, I benchmarked saving 1 million JsonDocuments to the database, recreating the objects each time without actually changing their content:

  • ~24 sec with the old comparer (unconditional save)
  • ~0,7 sec with the new comparer (save skipped — no actual change detected)

Relates to #3314

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves EFCore.PG change tracking for JSON DOM-mapped properties (JsonDocument, JsonElement) by introducing provider-specific ValueComparers that use JsonElement.DeepEquals, reducing unnecessary UPDATEs when JSON content is unchanged but object instances differ.

Changes:

  • Add ValueComparer implementations for JsonDocument and JsonElement to NpgsqlJsonTypeMapping, using deep JSON equality semantics.
  • Extend unit tests to validate deep equality behavior for the JSON DOM comparers.
  • Add functional tests asserting that SaveChanges emits UPDATEs only when JSON content changes (and does not throw for undefined JsonElement cases).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/EFCore.PG/Storage/Internal/Mapping/NpgsqlJsonTypeMapping.cs Adds JSON DOM ValueComparers and wires them into the JSON type mapping so change tracking compares by JSON content.
test/EFCore.PG.Tests/Storage/NpgsqlTypeMappingTest.cs Expands comparer unit tests to verify deep equality across new instances, ordering/number formatting cases, and null/undefined scenarios.
test/EFCore.PG.FunctionalTests/Query/JsonDomChangeTrackingTest.cs New functional coverage validating UPDATE/no-UPDATE behavior for json/jsonb DOM properties and ensuring undefined JsonElement doesn’t throw during DetectChanges.

Comment on lines +160 to +164
private sealed class JsonDocumentComparer() : ValueComparer<JsonDocument>(
(a, b) => a == null ? b == null : b != null && JsonElement.DeepEquals(a.RootElement, b.RootElement),
o => o.GetHashCode(),
// Disposing the original JsonDocument will throw when it's later compared against the snapshot
o => o);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't actually an issue. ValueComparer has the following constructor:

public ValueComparer(
Expression<Func<T?, T?, bool>> equalsExpression,
Expression<Func<T, int>> hashCodeExpression,
Expression<Func<T, T>> snapshotExpression)
: base(equalsExpression, hashCodeExpression, snapshotExpression)
{
}

Note that unlike equalsExpression, hashCodeExpression is typed as Func<T, int> rather than Func<T?, int>T isn't nullable here, so this is expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants