Skip to content

Conversation

@mrmarcosmagalhaes
Copy link

@mrmarcosmagalhaes mrmarcosmagalhaes commented Jul 16, 2025

Closes #1500

QUALITY CHECKLIST

@bkoelman bkoelman changed the base branch from request-meta-tests-boilerplate to master July 19, 2025 09:08
@bkoelman
Copy link
Member

Thanks, this looks great!

I've changed the base of this PR, so that status checks run.

@codecov
Copy link

codecov bot commented Jul 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.25%. Comparing base (506a855) to head (481fd15).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1746   +/-   ##
=======================================
  Coverage   92.24%   92.25%           
=======================================
  Files         437      437           
  Lines       14860    14860           
  Branches     2451     2451           
=======================================
+ Hits        13708    13709    +1     
+ Misses        709      708    -1     
  Partials      443      443           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bkoelman
Copy link
Member

bkoelman commented Dec 3, 2025

@mrmarcosmagalhaes, do you still want to complete the work on this PR? If not, please let me know, so I can pick up from here.

@mrmarcosmagalhaes
Copy link
Author

mrmarcosmagalhaes commented Dec 8, 2025 via email

@bkoelman
Copy link
Member

bkoelman commented Dec 9, 2025

Congratulations! Looking forward to your contribution.

@mrmarcosmagalhaes
Copy link
Author

@bkoelman Completed and consolidated RequestMetaTests.
All resource, relationship, and atomic operations now have a single integration test per endpoint, covering all valid meta locations.
Added full coverage for atomic relationship operations (to-one and to-many), fully aligned with the JSON:API spec.
Sorry for the late push but my life right now is not easy.

@mrmarcosmagalhaes
Copy link
Author

I only need to fix the code styles and maybe everything is ok
Can you review?
Thanks

Copy link
Member

@bkoelman bkoelman left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I've taken a closer look and left some additional feedback.

Please don't resolve open conversations; see https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/master/.github/CONTRIBUTING.md#pull-request-workflow.

Comment on lines 47 to 48
SupportTicket ticket = _fakers.SupportTicket.GenerateOne();
ProductFamily family = _fakers.ProductFamily.GenerateOne();
Copy link
Member

Choose a reason for hiding this comment

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

In tests that change data, we use the following naming conventions:

  • existingTicket: This is added to the database before the request executes.
  • newTicket: This is used to test creating a resource that doesn't exist yet, if multiple fields are used. However, if only a single field is used in the request (which is the case in these tests), instead use: string newTicketDescription = _fakers.SupportTicket.GenerateOne().Description
  • ticketInDatabase: This is used when asserting something changed in the database afterwards

We don't have a preference whether ticket or supportTicket is used as a name, but it should be consistent. Since existing tests already use ticket and family, I suggest aligning with that.

Please update existing names to match the above.

SupportTicket ticket = _fakers.SupportTicket.GenerateOne();
ProductFamily family = _fakers.ProductFamily.GenerateOne();

await _testContext.RunOnDatabaseAsync(async db =>
Copy link
Member

Choose a reason for hiding this comment

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

db should be renamed to dbContext to align with existing conventions.

}

[Fact]
public async Task Accepts_meta_in_post_resource_request_with_relationship()
Copy link
Member

Choose a reason for hiding this comment

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

There should be two tests: to-one and to-many, similar to the patch-resource cases.

}

[Fact]
public async Task Accepts_meta_in_delete_relationship_request()
Copy link
Member

Choose a reason for hiding this comment

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

For relationship endpoints, we should have the following tests:

  • Accepts_meta_in_update_to_one_relationship_request: PATCH /supportTickets/{id}/relationships/productFamily
  • Accepts_meta_in_update_to_many_relationship_request: PATCH /productFamilies/{id}/relationships/tickets
  • Accepts_meta_in_post_relationship_request (because this must always be to-many): POST /productFamilies/{id}/relationships/tickets
  • Accepts_meta_in_delete_relationship_request (because this must always be to-many): DELETE /productFamilies/{id}/relationships/tickets

}

[Fact]
public async Task Accepts_meta_in_atomic_update_resource_operation()
Copy link
Member

Choose a reason for hiding this comment

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

Please extract the tests for operations into a separate class OperationsRequestMetaTests. They should provide the same test scenarios as for resource/relationship endpoints, plus deletion of a resource. (Ideally, they would live in IntegrationTests/AtomicOperations/Meta, but then you'd need to switch to other models, which would be way more work, so I'm okay with just leaving them in this directory.)

I think it becomes easier to switch to functional names for the tests at this point. For example, instead of:
Accepts_meta_in_patch_resource_request_with_to_one_relationship (which describes the technical endpoint), it would become:
Accepts_meta_in_update_resource_request_with_to_one_relationship, so that the operations equivalent becomes:
Accepts_meta_in_update_resource_operation_with_to_one_relationship.

Likewise:
Accepts_meta_in_delete_relationship_request then becomes:
Accepts_meta_in_remove_from_to_many_relationship_request

Oh, and one more thing: I see that existing tests use ToOne and ToMany instead of to_one and to_many, so please align with that as well.

Comment on lines 290 to 294
await _testContext.RunOnDatabaseAsync(async db =>
{
SupportTicket updated = await db.SupportTickets.FirstAsync(supportTicket => supportTicket.Id == existingTicket.Id);
updated.Description.Should().Be(existingTicket.Description);
});
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it adds value to assert that the change was persisted in the database for these tests. The goal is to verify that the request meta is accessible at the server. While it's good to assert on the status code, so we'll know if the request failed (and the full server error is printed in that case), asserting on the request meta should be sufficient.

Comment on lines 408 to 414
IDictionary<string, RelationshipObject?>? relationships = operation.Data.SingleValue.Relationships;
relationships.Should().NotBeNull();
relationships.Should().ContainKey("productFamily");

RelationshipObject? relationship = relationships["productFamily"];
relationship.Should().NotBeNull();
relationship.Meta.Should().NotBeNull();
Copy link
Member

Choose a reason for hiding this comment

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

There's a more elegant way to assert on sub-structures, without the need to introduce top-level variables. For example, see here.

Comment on lines +198 to +263

// document meta explicit validation
store.Document.Meta.Should().HaveCount(documentMeta.Count);
store.Document.Meta.Should().ContainKey("requestId").WhoseValue.With(value =>
{
JsonElement element = value.Should().BeOfType<JsonElement>().Subject;
element.GetString().Should().Be((string)documentMeta["requestId"]);
});

// resource meta explicit validation
store.Document.Data.SingleValue.Should().NotBeNull();
store.Document.Data.SingleValue.Meta.Should().HaveCount(resourceMeta.Count);
store.Document.Data.SingleValue.Meta.Should().ContainKey("editedBy").WhoseValue.With(value =>
{
JsonElement element = value.Should().BeOfType<JsonElement>().Subject;
element.GetString().Should().Be((string)resourceMeta["editedBy"]);
});
store.Document.Data.SingleValue.Meta.Should().ContainKey("revision").WhoseValue.With(value =>
{
JsonElement element = value.Should().BeOfType<JsonElement>().Subject;
element.GetInt32().Should().Be((int)resourceMeta["revision"]);
});

// relationship meta validation
store.Document.Data.SingleValue.Relationships.Should().NotBeNull();
store.Document.Data.SingleValue.Relationships.Should().ContainKey("tickets").WhoseValue.With(value =>
{
value.Should().NotBeNull();
value.Meta.Should().HaveCount(relationshipMeta.Count);
value.Meta.Should().ContainKey("source").WhoseValue.With(val =>
{
JsonElement element = val.Should().BeOfType<JsonElement>().Subject;
element.GetString().Should().Be((string)relationshipMeta["source"]);
});
value.Meta.Should().ContainKey("confidence").WhoseValue.With(val =>
{
JsonElement element = val.Should().BeOfType<JsonElement>().Subject;
element.GetDouble().Should().BeApproximately((double)relationshipMeta["confidence"], 1e-6);
});

value.Data.ManyValue.Should().HaveCount(2);

value.Data.ManyValue[0].Meta.Should().HaveCount(identifierMeta1.Count);
value.Data.ManyValue[0].Meta.Should().ContainKey("index").WhoseValue.With(v =>
{
JsonElement element = v.Should().BeOfType<JsonElement>().Subject;
element.GetInt32().Should().Be((int)identifierMeta1["index"]);
});
value.Data.ManyValue[0].Meta.Should().ContainKey("optionalNote").WhoseValue.With(v =>
{
JsonElement element = v.Should().BeOfType<JsonElement>().Subject;
element.GetString().Should().Be((string)identifierMeta1["optionalNote"]);
});

value.Data.ManyValue[1].Meta.Should().HaveCount(identifierMeta2.Count);
value.Data.ManyValue[1].Meta.Should().ContainKey("index").WhoseValue.With(v =>
{
JsonElement element = v.Should().BeOfType<JsonElement>().Subject;
element.GetInt32().Should().Be((int)identifierMeta2["index"]);
});
value.Data.ManyValue[1].Meta.Should().ContainKey("optionalNote").WhoseValue.With(v =>
{
JsonElement element = v.Should().BeOfType<JsonElement>().Subject;
element.GetString().Should().Be((string)identifierMeta2["optionalNote"]);
});
});
Copy link
Member

Choose a reason for hiding this comment

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

Now that the actual meta content is produced by fakers (thereby hidden from the tests), I'd like to remove the dependency on what the fakers produce from the tests' assertions.

Suggested change
// document meta explicit validation
store.Document.Meta.Should().HaveCount(documentMeta.Count);
store.Document.Meta.Should().ContainKey("requestId").WhoseValue.With(value =>
{
JsonElement element = value.Should().BeOfType<JsonElement>().Subject;
element.GetString().Should().Be((string)documentMeta["requestId"]);
});
// resource meta explicit validation
store.Document.Data.SingleValue.Should().NotBeNull();
store.Document.Data.SingleValue.Meta.Should().HaveCount(resourceMeta.Count);
store.Document.Data.SingleValue.Meta.Should().ContainKey("editedBy").WhoseValue.With(value =>
{
JsonElement element = value.Should().BeOfType<JsonElement>().Subject;
element.GetString().Should().Be((string)resourceMeta["editedBy"]);
});
store.Document.Data.SingleValue.Meta.Should().ContainKey("revision").WhoseValue.With(value =>
{
JsonElement element = value.Should().BeOfType<JsonElement>().Subject;
element.GetInt32().Should().Be((int)resourceMeta["revision"]);
});
// relationship meta validation
store.Document.Data.SingleValue.Relationships.Should().NotBeNull();
store.Document.Data.SingleValue.Relationships.Should().ContainKey("tickets").WhoseValue.With(value =>
{
value.Should().NotBeNull();
value.Meta.Should().HaveCount(relationshipMeta.Count);
value.Meta.Should().ContainKey("source").WhoseValue.With(val =>
{
JsonElement element = val.Should().BeOfType<JsonElement>().Subject;
element.GetString().Should().Be((string)relationshipMeta["source"]);
});
value.Meta.Should().ContainKey("confidence").WhoseValue.With(val =>
{
JsonElement element = val.Should().BeOfType<JsonElement>().Subject;
element.GetDouble().Should().BeApproximately((double)relationshipMeta["confidence"], 1e-6);
});
value.Data.ManyValue.Should().HaveCount(2);
value.Data.ManyValue[0].Meta.Should().HaveCount(identifierMeta1.Count);
value.Data.ManyValue[0].Meta.Should().ContainKey("index").WhoseValue.With(v =>
{
JsonElement element = v.Should().BeOfType<JsonElement>().Subject;
element.GetInt32().Should().Be((int)identifierMeta1["index"]);
});
value.Data.ManyValue[0].Meta.Should().ContainKey("optionalNote").WhoseValue.With(v =>
{
JsonElement element = v.Should().BeOfType<JsonElement>().Subject;
element.GetString().Should().Be((string)identifierMeta1["optionalNote"]);
});
value.Data.ManyValue[1].Meta.Should().HaveCount(identifierMeta2.Count);
value.Data.ManyValue[1].Meta.Should().ContainKey("index").WhoseValue.With(v =>
{
JsonElement element = v.Should().BeOfType<JsonElement>().Subject;
element.GetInt32().Should().Be((int)identifierMeta2["index"]);
});
value.Data.ManyValue[1].Meta.Should().ContainKey("optionalNote").WhoseValue.With(v =>
{
JsonElement element = v.Should().BeOfType<JsonElement>().Subject;
element.GetString().Should().Be((string)identifierMeta2["optionalNote"]);
});
});
store.Document.Meta.Should().BeEquivalentToJson(documentMeta);
store.Document.Data.SingleValue.Should().NotBeNull();
store.Document.Data.SingleValue.Meta.Should().BeEquivalentToJson(resourceMeta);
store.Document.Data.SingleValue.Relationships.Should().NotBeNull();
store.Document.Data.SingleValue.Relationships.Should().ContainKey("tickets").WhoseValue.With(value =>
{
value.Should().NotBeNull();
value.Meta.Should().BeEquivalentToJson(relationshipMeta);
value.Data.ManyValue.Should().HaveCount(2);
value.Data.ManyValue[0].Meta.Should().BeEquivalentToJson(identifierMeta1);
value.Data.ManyValue[1].Meta.Should().BeEquivalentToJson(identifierMeta2);
});

This code requires adding the following to FluentMetaExtensions.cs:

    private static readonly JsonSerializerOptions MetaSerializerOptions = new()
    {
        WriteIndented = true,
        Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
        ReferenceHandler = ReferenceHandler.IgnoreCycles
    };

    /// <summary>
    /// Asserts that the content of a "meta" dictionary matches the expected structure and values, after conversion to JSON.
    /// </summary>
    [CustomAssertion]
    public static void BeEquivalentToJson(this GenericDictionaryAssertions<IDictionary<string, object?>, string, object?> source,
        Dictionary<string, object?> expected)
    {
        source.NotBeNull();

        string sourceJson = JsonSerializer.Serialize(source.Subject, MetaSerializerOptions);
        string expectedJson = JsonSerializer.Serialize(expected, MetaSerializerOptions);

        sourceJson.Should().Be(expectedJson);
    }

This requires changing the return type from Dictionary<string, object> to Dictionary<string, object?> in fakers.

// Arrange
var store = _testContext.Factory.Services.GetRequiredService<RequestDocumentStore>();

var documentMeta = _fakers.DocumentMeta.Generate();
Copy link
Member

Choose a reason for hiding this comment

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

Please use .GenerateOne(), so that the proper nullability is inferred.

{1C872180-7A7E-46F0-8144-891A45CB514A}

[Fact]
public async Task Accepts_meta_in_patch_resource_request_with_to_one_relationship()
{
// Assert
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Assert
// Arrange

[Fact]
public async Task Accepts_meta_in_update_to_one_relationship_operation()
{
var store = _testContext.Factory.Services.GetRequiredService<RequestDocumentStore>();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var store = _testContext.Factory.Services.GetRequiredService<RequestDocumentStore>();
// Arrange
var store = _testContext.Factory.Services.GetRequiredService<RequestDocumentStore>();


const string route = "/operations";

(HttpResponseMessage httpResponse, _) = await _testContext.ExecutePostAtomicAsync<Document>(route, requestBody);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(HttpResponseMessage httpResponse, _) = await _testContext.ExecutePostAtomicAsync<Document>(route, requestBody);
// Act
(HttpResponseMessage httpResponse, _) = await _testContext.ExecutePostAtomicAsync<Document>(route, requestBody);


(HttpResponseMessage httpResponse, _) = await _testContext.ExecutePostAtomicAsync<Document>(route, requestBody);

httpResponse.ShouldHaveStatusCode(HttpStatusCode.NoContent);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
httpResponse.ShouldHaveStatusCode(HttpStatusCode.NoContent);
// Assert
httpResponse.ShouldHaveStatusCode(HttpStatusCode.NoContent);


store.Document.Should().NotBeNull();

// document meta explicit validation
Copy link
Member

Choose a reason for hiding this comment

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

Please remove these comments. With the proposed change to use BeEquivalentToJson, they should no longer be needed to understand what the test does. When comments are needed to explain something, it triggers me to think whether the code could be made easier to understand instead.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Add tests for submitting meta

2 participants