Add example projects and tests

[mrnkr]

This PR is what @bart-degreed had suggested in #2 , let me quote for reference:

Add checks to the MongoDB repository that fail with an error whenever relationships are used. And add the example and test projects. But instead of commented-out tests, this would contain tests that assert an appropriate error is returned when using relationships. From what I read online, it looks like MongoDB requires IDs to always be strings. So we'll also need a test covering that. And there may be other corner cases to catch, such as the nullable type conversion. This PR would pin the current behavior, identifying what works and what does not.

Closes #1.

[bart-degreed]

Please merge in latest master, I've setup cibuild there. And I created a v4-alpha1 release on GitHub, to test that it automatically publishes to NuGet.

I understand you don't want to make too many changes in this PR. The way I'm going to review the tests in this PR, is by running a file-by-file diff with the main repo. For each test, I'm going to ask myself questions like: Why are the assertions different here? Why was this test removed? What other cases should be added? Is this a duplicate? Is this testing serializer behavior, so it should be left out?

My point is that reviewing a large set with gaps (future work) is a lot harder than reviewing a set that is supposed to be complete. And once the set is complete, I'm going to ask you to remove several models, resource hooks etc, because they are unneeded.

I don't think this needs to result in a lot of changes on your side. For example, we have many tests for usage of the ?include= query string parameter. You can substitute that whole set with a single test that fails whenever ?include= is used. You'd just throw a JsonApiException with title "Relationships are not supported when using MongoDB." from the repository ApplyQueryLayer method whenever QueryLayer.Include is not null. Also throw when QueryLayer.Projection contains relationships. And there's a bit more to check, but you'll find them when adding such tests and I can help you how to fix that.

[mrnkr]

I've just added error messages for when the user attempts to fetch a related resource (either via ?include= or just /primary/:id/secondary) and for resource creation as well.

I also went back to the tests I'd said I was failing in #2 and I have some things to comment:

• Comparing attributes is indeed not supported by MongoDB.Driver https://jira.mongodb.org/browse/CSHARP-1592
• I left the test Retrieves_all_properties_when_fieldset_contains_readonly_attribute with a different name and asserting a 500 status code because in order to fix it I would need to actually implement IModel.GetEntityTypes() in DummyModel, right now it just throws NotImplementedException. Do you think there is a way for me to catch this case before it gets there so I can throw a proper error earlier?

As for resource updates I couldn't figure out what it would be like to test that because since you can't create how do you even think of updating, right? I understand we will most likely want to test this a bit anyway so some suggestions would come in handy!

[bart-degreed]

Retrieves_all_properties_when_fieldset_contains_readonly_attribute

To make the original test work, you just need to feed it the info SelectClauseBuilder is asking for. The code below replaces the static Dummy instance with an implementation that performs the lookup through the resource graph. It's mostly boilerplate from interface members.

MongoDbModel source code

using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using JsonApiDotNetCore.Configuration;
using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.EntityFrameworkCore.Metadata;

namespace JsonApiDotNetCore.MongoDb
{
    internal sealed class MongoDbModel : IModel
    {
        private readonly IResourceContextProvider _resourceContextProvider;

        public object this[string name] => throw new NotImplementedException();

        public MongoDbModel(IResourceContextProvider resourceContextProvider)
        {
            _resourceContextProvider = resourceContextProvider ?? throw new ArgumentNullException(nameof(resourceContextProvider));
        }

        public IEnumerable<IEntityType> GetEntityTypes()
        {
            var resourceContexts = _resourceContextProvider.GetResourceContexts();
            return resourceContexts.Select(resourceContext => new MongoEntityType(resourceContext, this));
        }

        public IAnnotation FindAnnotation(string name) => throw new NotImplementedException();
        public IEnumerable<IAnnotation> GetAnnotations() => throw new NotImplementedException();
        public IEntityType FindEntityType(string name) => throw new NotImplementedException();
        public IEntityType FindEntityType(string name, string definingNavigationName, IEntityType definingEntityType) => throw new NotImplementedException();
    }

    internal sealed class MongoEntityType : IEntityType
    {
        private readonly ResourceContext _resourceContext;

        public IModel Model { get; }
        public Type ClrType => _resourceContext.ResourceType;

        public string Name => throw new NotImplementedException();
        public IEntityType BaseType => throw new NotImplementedException();
        public string DefiningNavigationName => throw new NotImplementedException();
        public IEntityType DefiningEntityType => throw new NotImplementedException();
        public object this[string name] => throw new NotImplementedException();

        public MongoEntityType(ResourceContext resourceContext, MongoDbModel owner)
        {
            _resourceContext = resourceContext ?? throw new ArgumentNullException(nameof(resourceContext));
            Model = owner ?? throw new ArgumentNullException(nameof(owner));
        }

        public IEnumerable<IProperty> GetProperties()
        {
            return _resourceContext.Attributes.Select(attr => new MongoDbProperty(attr.Property, this));
        }

        public IAnnotation FindAnnotation(string name) => throw new NotImplementedException();
        public IEnumerable<IAnnotation> GetAnnotations() => throw new NotImplementedException();
        public IKey FindPrimaryKey() => throw new NotImplementedException();
        public IKey FindKey(IReadOnlyList<IProperty> properties) => throw new NotImplementedException();
        public IEnumerable<IKey> GetKeys() => throw new NotImplementedException();
        public IForeignKey FindForeignKey(IReadOnlyList<IProperty> properties, IKey principalKey, IEntityType principalEntityType) => throw new NotImplementedException();
        public IEnumerable<IForeignKey> GetForeignKeys() => throw new NotImplementedException();
        public IIndex FindIndex(IReadOnlyList<IProperty> properties) => throw new NotImplementedException();
        public IEnumerable<IIndex> GetIndexes() => throw new NotImplementedException();
        public IProperty FindProperty(string name) => throw new NotImplementedException();
        public IServiceProperty FindServiceProperty(string name) => throw new NotImplementedException();
        public IEnumerable<IServiceProperty> GetServiceProperties() => throw new NotImplementedException();
    }

    internal sealed class MongoDbProperty : IProperty
    {
        public IEntityType DeclaringEntityType { get; }
        public PropertyInfo PropertyInfo { get; }

        public string Name => throw new NotImplementedException();
        public Type ClrType => throw new NotImplementedException();
        public FieldInfo FieldInfo => throw new NotImplementedException();
        public ITypeBase DeclaringType => throw new NotImplementedException();
        public bool IsNullable => throw new NotImplementedException();
        public ValueGenerated ValueGenerated => throw new NotImplementedException();
        public bool IsConcurrencyToken => throw new NotImplementedException();
        public object this[string name] => throw new NotImplementedException();

        public MongoDbProperty(PropertyInfo propertyInfo, MongoEntityType owner)
        {
            DeclaringEntityType = owner ?? throw new ArgumentNullException(nameof(owner));
            PropertyInfo = propertyInfo ?? throw new ArgumentNullException(nameof(propertyInfo));
        }

        public IAnnotation FindAnnotation(string name) => throw new NotImplementedException();
        public IEnumerable<IAnnotation> GetAnnotations() => throw new NotImplementedException();
    }
}

Please put each of these classes in its own file. We prefer multiple types in a single file only when nested or only their generic arguments vary.

[bart-degreed]

As for resource updates I couldn't figure out what it would be like to test that because since you can't create how do you even think of updating, right? I understand we will most likely want to test this a bit anyway so some suggestions would come in handy!

The use case is that an API developer can define resources with HasOne/HasMany/HasManyThrough attributes on them. And then his/her customer can send a GET/POST/PATCH that refers to those relationships.

My first thought to block relationships was to just forbid using these attributes in resources. This repo could add a post-processor step in the resource graph building process that throws when relationships are modelled. But that is too restrictive, because an API may serve both EF Core and MongoDB resources at the same time.

The only place where we run into trouble, is when the code enters MongoDbRepository, so that is the place to intercept.

So to answer your question: A test that shows PATCH fails when relationships are being sent in the request body would not actually need to create a resource with a relationship in the test-database upfront (there is currently no way to do that). Not a problem, because the API code never gets there. But the resource class being used in the test does need to define a relationship. Then the code passes through query string parsing without errors, ending up in the repo.

Clarification: With "the resource class being used in the test does need to define a relationship" I mean it needs to contain a HasOne/HasMany/HasManyThrough attribute. There is no need to add MongoDB-specific attributes to make it persist as a relationship. For all I know, relationships do not exist in MongoDB, right? :)

[bart-degreed]

I've taken a peek at your changes in MongoDbRepository and couldn't resist trying to help a bit to get it right. Doing so requires deep knowledge of the various expressions and how they compose. Since I've built that and its still fresh in my memory I was able to come up with the validator below within an hour. I Hope I didn't spoil the fun for you, please let me know if you want me to hold back a bit more.

MongoDbQueryExpressionValidator source code

internal sealed class MongoDbQueryExpressionValidator : QueryExpressionRewriter<object>
{
    public void Validate(QueryLayer layer)
    {
        if (layer == null) throw new ArgumentNullException(nameof(layer));

        bool hasIncludes = layer.Include?.Elements.Any() == true;
        var hasSparseRelationshipSets = layer.Projection?.Any(pair => pair.Key is RelationshipAttribute) == true;

        if (hasIncludes || hasSparseRelationshipSets)
        {
            throw CreateErrorForRelationshipUsage();
        }

        ValidateExpression(layer.Filter);
        ValidateExpression(layer.Sort);
        ValidateExpression(layer.Pagination);
    }

    private static JsonApiException CreateErrorForRelationshipUsage()
    {
        return new JsonApiException(new Error(HttpStatusCode.BadRequest)
        {
            Title = "Relationships are not supported when using MongoDB."
        });
    }

    private void ValidateExpression(QueryExpression expression)
    {
        if (expression != null)
        {
            Visit(expression, null);
        }
    }

    public override QueryExpression VisitResourceFieldChain(ResourceFieldChainExpression expression, object argument)
    {
        if (expression != null)
        {
            if (expression.Fields.Count > 1 || expression.Fields.First() is RelationshipAttribute)
            {
                throw CreateErrorForRelationshipUsage();
            }
        }

        return base.VisitResourceFieldChain(expression, argument);
    }

    public override QueryExpression VisitComparison(ComparisonExpression expression, object argument)
    {
        if (expression?.Left is ResourceFieldChainExpression && expression.Right is ResourceFieldChainExpression)
        {
            // https://jira.mongodb.org/browse/CSHARP-1592
            throw new JsonApiException(new Error(HttpStatusCode.BadRequest)
            {
                Title = "Comparing attributes against each other is not supported when using MongoDB."
            });
        }

        return base.VisitComparison(expression, argument);
    }
}

You can call Validate() from MongoDbRepository.ApplyQueryLayer, just after its parameter null assertion. Then you can remove your custom catch block in GetAsync, AssertNoInclude etc.

I noticed you were adding the "include" parameter to the exception, which is fine for include, but hard to reconstruct for other query string parameters such as fields[]. So I took that out, it is not important enough to do extra work for it.

Also I created a private method to produce the exception when a relationship is encountered. You'll likely also need that from within MongoDbRepository, so I think its best to create a custom exception for that. See the exceptions in JsonApiDotNetCore.Errors namespace in the main repo for inspiration.

[bart-degreed]

Review of your last commit only. Please let me know when you're ready for review of the rest.

[mrnkr]

Please don't hold back on my account! Thank you so much for those snippets, they worked great! Knowing the codebase does help when coming up with solutions to these problems hehe.

Anyway, I just added a test for updating relationships, cleaned up the models that had to have relationships and addressed the comments of your review. I think I'm ready for a review of the entire PR.

[bart-degreed]

Reviewed until Meta tests, will look at the rest later.

[bart-degreed]

@mrnkr You mentioned here that resource metadata is broken. Can you elaborate on that?

[mrnkr]

@mrnkr You mentioned here that resource metadata is broken. Can you elaborate on that?

Yes, I'd forgotten about that... The test I was failing was ResourceMetaTests.ResourceDefinition_That_Implements_GetMeta_Contains_Resource_Meta with the following exception:

JsonApiDotNetCore.MongoDb.Example.Tests.IntegrationTests.Meta.ResourceMetaTests.ResourceDefinition_That_Implements_GetMeta_Contains_Resource_Meta

Xunit.Sdk.XunitException
Expected responseDocument.ManyData[0].Meta to contain keys {"hasHighPriority"}, but found <null>.
   at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
   at FluentAssertions.Execution.DefaultAssertionStrategy.HandleFailure(String message)
   at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
   at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
   at FluentAssertions.Execution.AssertionScope.FailWith(String message, Object[] args)
   at FluentAssertions.Collections.GenericDictionaryAssertions`2.ContainKeys(IEnumerable`1 expected, String because, Object[] becauseArgs)
   at FluentAssertions.Collections.GenericDictionaryAssertions`2.ContainKey(TKey expected, String because, Object[] becauseArgs)
   at JsonApiDotNetCore.MongoDb.Example.Tests.IntegrationTests.Meta.ResourceMetaTests.ResourceDefinition_That_Implements_GetMeta_Contains_Resource_Meta() in /Users/mrnkr/Documents/code/JsonApiDotNetCore.MongoDb/test/JsonApiDotNetCore.MongoDb.Example.Tests/IntegrationTests/Meta/ResourceMetaTests.cs:line 53
   at Xunit.Sdk.TestInvoker`1.<>c__DisplayClass48_1.<<InvokeTestMethodAsync>b__1>d.MoveNext() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\Runners\TestInvoker.cs:line 264
--- End of stack trace from previous location where exception was thrown ---
   at Xunit.Sdk.ExecutionTimer.AggregateAsync(Func`1 asyncAction) in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\ExecutionTimer.cs:line 48
   at Xunit.Sdk.ExceptionAggregator.RunAsync(Func`1 code) in C:\Dev\xunit\xunit\src\xunit.core\Sdk\ExceptionAggregator.cs:line 90

Basically, the metadata just isn't added to the response object.

[bart-degreed]

I've gone over the open feedback items today, and noticed that some of them aren't yet addressed, others only partly.

So I'd like to structure our way of working a bit, as it is becoming unclear to me what is going on. Let's follow these steps:

1. When you're ready for another review round, use the GitHub button to re-request review
2. While reviewing, I'm adding new feedback items
3. Once you've addressed a feedback item, mark it with a thumbs-up -or- comment so we can discuss
4. I'll verify and mark addressed feedback items as resolved

[mrnkr]

Sorry things got a bit confusing... I think I did get all your comments sorted out this time, if not I did leave comments.

You'll notice I'm a bit lost in terms of how to proceed in terms of the options.SerializerSettings.DateFormatString issue and resource metadata. Aside from that I think I'm ready for the next review round.

[mrnkr]

Left the new tests for ReadWrite for tomorrow and left a couple questions.

[bart-degreed]

Gone over open items, think I addressed all your questions.

[bart-degreed]

Skimmed over the ReadWrite tests quickly, I'll take a closer look once you're done with them.

[mrnkr]

I've just finished adding all the tests for ReadWrite that seemed relevant according to what you've told me in previous comments. I've also addressed all the feedback items so I requested another review round.

[bart-degreed]

I've gone over all ReadWrite tests now.

[bart-degreed]

This should be named Can_create_resource_with_string_ID. Assuming MongoDB only works with string IDs, there should also be a test like Cannot_create_resource_with_int_ID.

[mrnkr]

Addressed all comments but two for which I left a question

[mrnkr]

Something else I forgot to mention, in adding the CreateResourceWithClientGeneratedIdTests class I saw I hadn't handled duplicate ID errors.

I saw in the main repo that exception is thrown from the service only and not from the repo, which I think throws a DataStoreUpdateException. Must have been my mistake but I couldn't make that work today... Did I overlook some key detail?

[bart-degreed]

Reviewed changes and added comments. Pushed commits to fix relationship links and duplicate IDs. The service catches DataStoreUpdateException when a write fails, so the repository must throw that. By using this exception, the service does not need to know about EF Core- or MongoDB-specific exception types.

[bart-degreed]

I think this is ready for merge now. Will hold off until I hear from you, in case I missed something. After merge, I intend to publish another alpha to NuGet.

[mrnkr]

I'm just taking the liberty to rename src/JsonApiDotNetCore.MongoDb/Configuration/public static class ServiceCollectionExtensions.cs to src/JsonApiDotNetCore.MongoDb/Configuration/ServiceCollectionExtensions.cs hehe

Other than that I'm happy to have this merged :)

[bart-degreed]

Ah, good catch!

Thanks a lot for your continued efforts working on this!