Return 404 or 200 on error/non existent resource

[wisepotato]

Closes #631

Problem

Upon getting to a resource that is non-existent, the following object is returned:

{
  "links": {
    "self": "https://localhost/api/users"
  },
  "data": null
}

Proposed solution

When a 200 is applicable, i.e. the route exists, but nothing can be returned, i.e. when a user with id 1 exists and a GET request is sent to http://example.com/users/1/books. But it should return a 404 if user 1 does not exist, also for http://example.com/users/1.

We also require a descriptive error message, instead of just returning 404.

Done

• Requesting an existing base model with no to-one relationships should return 200 with null for data. ( GET /api/v1/todoItems/412/oneToOnePerson)
• [] Requesting an existing base model's relationships with a to-many relationship should return 200 with [] for data ( GET /api/v1/todoItems/1/stakeHolders)
• Requesting an existing base model with no to-one relationships should return 200 with null for data. ( GET /api/v1/todoItems/412/relationships/oneToOnePerson)
• Requesting an existing base model's relationships with a to-many relationship should return 200 with [] for data ( GET /api/v1/todoItems/1/relationships/stakeHolders)
• Requesting an non-existing base model (todoItem) should return 404

[wisepotato]

@bart-degreed anything you're missing?

[bart-degreed]

See comments. I only tested getting a single missing item, for example /books/123.

[bart-degreed]

The resource of type...
and should "id" be uppercase?

[bart-degreed]

This check is duplicated in both true and false blocks of the outer if statement. I believe it can be moved to the top, outside the if clauses, so it appears only once.
They are actually different: Relationship vs Relationships

[bart-degreed]

This type appears to be unused and can be removed.

[bart-degreed]

The private field that is assigned from this parameter is never used.

[bart-degreed]

I think there should be a space between the if keyword and the opening parenthesis.

[bart-degreed]

Unused variable

[bart-degreed]

Unclear comment

[bart-degreed]

Expression is always true. And should test for IEnumerable instead of IList.

[bart-degreed]

Why was this block commented out? Should it be removed instead?

[bart-degreed]

I did some more testing today and found that this PR throws a NullReferenceException on many-to-many relationships in DefaultResourceService at the next line:

var relationshipValue = typeof(TResource).GetProperty(relationship.InternalRelationshipName).GetValue(entity) ;

in the debugger, relationship.InternalRelationshipName has value JsonTags (which is the json-exposed property, not what is backed by an EF Core entity. My model looks like this:

public sealed class Pathway : Identifiable
{
    [NotMapped]
    [HasManyThrough("tags", nameof(Tags))]
    public List<Tag> JsonTags { get; set; }

    public List<PathwayTag> Tags { get; set; }
}

where PathwayTag is the join-table and Tag the exposed resource. Used URL:

https://localhost/pathways/ABC/tags

So I tried to fix this by adding this:

            object relationshipValue;
            if (relationship is HasManyThroughAttribute hasManyThroughRelationship)
            {
                relationshipValue = hasManyThroughRelationship.ThroughProperty.GetValue(entity);
            }
            else
            {
                relationshipValue = typeof(TResource).GetProperty(relationship.InternalRelationshipName).GetValue(entity);
            }

this works, unless there is no match found. In that case, this method returns null, which causes another NullReferenceException raised at the last line of:

        // triggered by GET /articles/1/{relationshipName}
        public virtual async Task<object> GetRelationshipAsync(TId id, string relationshipName)
        {
            var relationship = GetRelationship(relationshipName);
            var resource = await GetRelationshipsAsync(id, relationshipName);
            return relationship.GetValue(resource);
        }

because resource is null, so getting the relationship value fails.

[bart-degreed]

Hi @wisepotato I believe I fixed all issues as part of #714. See the tests in FetchingRelationshipsTests and the last two of UpdatingRelationshipsTests. Would you be okay if we close this PR? If I've missed anything, please let me know.