Prepare for Beta Release #2

mrnkr · 2020-11-02T01:53:33Z

I did as much as I could to get the project closer to what is proposed in issue #1 - I made the project compile using the latest version of dotnet core, did some integration tests (I'll need some help getting to 100% coverage, I'll admit) and I added an example project.

What's missing from that checklist?

Travis does not deploy the package tu NuGet, it just runs tests for now.
Haven't even thought about versioning yet.

I really want this to be published so I hope with this and a bit more effort we'll get there!

Let me know if you see anything I could make better.

…ceGraph that works

codecov-io · 2020-11-02T01:55:42Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a9e8a68). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master       #2   +/-   ##
=========================================
  Coverage          ?   78.57%           
=========================================
  Files             ?        2           
  Lines             ?       98           
  Branches          ?        6           
=========================================
  Hits              ?       77           
  Misses            ?       17           
  Partials          ?        4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9e8a68...a416207. Read the comment docs.

first release

alastairtree · 2020-12-12T17:35:40Z

This is great - have tested the basics and seems to work well. I had to comment out FlushFromCache throwing an exception as that was failing on Patch requests but otherwise the basics of GET POST PATCH and pagination seemed to work really well. What is required to get this merged/completed?

bart-degreed · 2020-12-14T09:43:34Z

Hi @alastairtree thanks for alerting us about this PR, I was unaware of it. It looks promising and I'm willing to provide review feedback in order to meet the same level of quality as the main project.

maurei · 2020-12-14T09:56:38Z

Hi @alastairtree, @mrnkr. Thanks for getting started on this project! A lot has changed in the main library since #1 was opened. First thing that comes to my mind using the extensively updated integration test suite in the main project to investigate more thoroughly what needs to be done to get the extension closer to a release. There is still some work to be done. Eg I noticed the mongodb repository is still inheriting from an old IResourceRepository interface (for example FlushFromCache was removed).

We might want to consider to put this extension in the main library for now, considering that we also don't have a JsonApiDotNetCore.EntityFrameworkCore package. But I'm unsure about this.

bart-degreed

@mrnkr Thanks a lot for your contribution! We're going to release JADNC v4-final soon, so it would be great if you can update to that. I've added several inline comments, would be great if you can take a look. Feel free to speak up if you disagree with anything.

bart-degreed · 2020-12-14T09:46:15Z

.gitignore

+# Created by https://www.toptal.com/developers/gitignore/api/dotnetcore,vscode,macos,linux,windows
+# Edit at https://www.toptal.com/developers/gitignore?templates=dotnetcore,vscode,macos,linux,windows


Can you copy the .gitignore from the main project instead? It has rules to exclude Rider caching etc.

bart-degreed · 2020-12-14T09:48:07Z

Directory.Build.props

+  <PropertyGroup>
+    <NetCoreAppVersion>netcoreapp3.1</NetCoreAppVersion>
+    <AspNetCoreVersion>3.1.*</AspNetCoreVersion>
+    <JsonApiDotNetCoreVersion>4.0.0-beta1</JsonApiDotNetCoreVersion>


bart-degreed · 2020-12-14T09:48:48Z

Directory.Build.props

+    <NetCoreAppVersion>netcoreapp3.1</NetCoreAppVersion>
+    <AspNetCoreVersion>3.1.*</AspNetCoreVersion>
+    <JsonApiDotNetCoreVersion>4.0.0-beta1</JsonApiDotNetCoreVersion>
+    <MongoDBDriverVersion>2.11.2</MongoDBDriverVersion>


Maybe add a wildcard in the version here too?

bart-degreed · 2020-12-14T09:50:02Z

JsonApiDotNetCore.MongoDb.sln

+MinimumVisualStudioVersion = 15.0.26124.0
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{7E29AA10-F938-4CF8-9CAB-7ACD2D6DC784}"
+EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Example", "src\Example\Example.csproj", "{600A3E66-E63F-427D-A991-4CD2067041F9}"


Suggestion for project rename: JsonApiDotNetCoreMongoDbExample

bart-degreed · 2020-12-14T09:53:19Z

JsonApiDotNetCore.MongoDb.sln

+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "JsonApiDotNetCore.MongoDb.IntegrationTests", "test\JsonApiDotNetCore.MongoDb.IntegrationTests\JsonApiDotNetCore.MongoDb.IntegrationTests.csproj", "{5C59FDFE-F079-4015-9975-FEFA766F0787}"
+EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "JsonApiDotNetCore.MongoDb.Example.Tests", "test\JsonApiDotNetCore.MongoDb.Example.Tests\JsonApiDotNetCore.MongoDb.Example.Tests.csproj", "{24CE53FA-9C49-4E20-A060-4A43DFB8C8F1}"
+EndProject


It is unclear to me why both projects use IntegrationTest namespace. It looks like the second one contains only unit tests using mocks, so JsonApiDotNetCore.MongoDb.UnitTests sounds more appropriate to me.

Actually the one I called JsonApiDotNetCore.MongoDb.IntegrationTests does connect to a MongoDb instance to run so it qualifies as integration tests, right? At least that was the reasoning behind my naming the project that way.

Integration testing spins up a webserver, sends a HTTP request to that and asserts on the HTTP response. To make that work efficiently, a light-weight or in-memory webserver/database is often used.

A typical integration test in JsonApiDotNetCore looks like the following:

setup infrastructure (base class or fixture)

seed database with predefined data

execute request

assert on response (match with what we put in the database upfront)

optional: if changes are expected, assert on changed rows in database

Aside to that, we have (a limited set of) unit tests. They test a single class (or a group of closely related classes) in isolation. For example, parsing a query string into expression objects. This is useful when there are many combinations to try (faster). The downside is it may be testing logic that is never used that way in the request execution pipeline, giving a false sense of quality.

bart-degreed · 2020-12-14T10:36:24Z

src/JsonApiDotNetCore.MongoDb/Queries/Internal/QueryableBuilding/QueryableBuilder.cs

+    /// <summary>
+    /// Drives conversion from <see cref="QueryLayer"/> into system <see cref="Expression"/> trees.
+    /// </summary>
+    public sealed class QueryableBuilder


Instead of copy/pasting this class, you can set layer.Projection = null before calling the built-in version to achieve the same result.

I tried this, it sounds like the right thing to do, but I run into the same problem I had when I was trying to use projections... In this case I can narrow it down to one parameter that's required by QueryableBuilder's constructor which is IModel model with IModel being exposed by EFCore.

What can we do about this??

bart-degreed · 2020-12-14T10:39:30Z

test/JsonApiDotNetCore.MongoDb.Example.Tests/IntegrationTestContext.cs

+        private string SerializeRequest(object requestBody) => 
+            requestBody is string stringRequestBody
+                ? stringRequestBody
+                : JsonConvert.SerializeObject(requestBody);


We recently fixed a bug that would send request body "null" on GET requests, which I think you should take (see json-api-dotnet/JsonApiDotNetCore#897).

bart-degreed · 2020-12-14T10:42:17Z

test/JsonApiDotNetCore.MongoDb.Example.Tests/IntegrationTests/CreatingResourcesTests.cs

+        {
+            var route = "/api/Books";
+            var book = _bookFaker.Generate();
+            var resource = new


This is not a resource, it is a request body. So I think this var should be renamed to requestBody instead.

bart-degreed · 2020-12-14T10:44:07Z

test/JsonApiDotNetCore.MongoDb.Example.Tests/IntegrationTests/CreatingResourcesTests.cs

+            var (httpResponse, responseDocument) = await _testContext.ExecutePostAsync<Document>(route, resource);
+            _createdBookId = responseDocument.Data is ResourceObject resourceObject ? resourceObject.Id : null;
+
+            Assert.Equal(HttpStatusCode.Created, httpResponse.StatusCode);


We're in the process of adopting FluentAssertions, would be great if you can use that too. It produces clearer failure messages and abstracts from xUnit syntax.

To check for status code, we have a custom extension that dumps the full response on mismatch.

bart-degreed · 2020-12-14T10:50:39Z

test/JsonApiDotNetCore.MongoDb.Example.Tests/IntegrationTests/DeletingResourcesTests.cs

+
+namespace JsonApiDotNetCore.MongoDb.Example.Tests.IntegrationTests
+{
+    public sealed class DeletingResourcesTests : IClassFixture<IntegrationTestContext<Startup>>, IAsyncLifetime


We've recently done major work on improving the tests for create/update/delete, so we now have over 200 integration tests for that.

I think you'd benefit from taking a subset of them. A great amount can be left out, because they check on invalid request bodies. And there are lots of relationship tests.

But others, like delete-not-found, create-already-exists etc. are things you need to handle properly for MongoDb, so this repo would benefit from having those tests.

mrnkr · 2020-12-14T13:10:00Z

@bart-degreed @maurei Thank you for all your feedback! I'll go through all the comments as soon as I can. @alastairtree thanks for bringing this PR back to life, I'd forgotten about it myself 😂

alastairtree · 2020-12-14T16:20:13Z

In terms of integration tests I really recommend mongo2go, it's on nuget and gives you a cross platform local test instance of the full db. Have used it lots and it is pretty useful.

mrnkr · 2020-12-18T00:29:28Z

I started working on the comments just now, most of them I've already taken care of but I broke all my tests so I'm obviously fixing that before I merge to master and update this PR.

mrnkr · 2020-12-24T22:19:02Z

Hello everyone, just wanted to leave you guys an update on this.

I have taken 175 tests from the JADNC repo and 168 of them are passing already. The reimplementation of the repo with the new version of the library is ready too, no relations support yet but all the rest is done.

Basically what I have left to figure out is support for filtering with variables, like /filterableResources?filter=equals(someNullableInt32,someInt32), resource metadata is broken in my implementation and I'm not sure why and, last but not least, I'm failing one test for sparse fieldsets because my implementation of IModel is just a dummy and the test I'm failing requires it not to be (test is Retrieves_all_properties_when_fieldset_contains_readonly_attribute).

All this is in my fork, in the branch fix/review_fixes and I'll get it merged as soon as I get these last few things right.

alastairtree · 2020-12-24T22:43:41Z

Thank you for the update! Sounds really promising. Happy holidays.

* wip: review fixes - all e2e tests broken * fix: change gitignore * fix: e2e tests * tests compiling but failing * fixed 35 tests * 70 tests passing - 4 to go * added sorting tests * fix: Proper Startup references to fix tests * added sparse fieldset tests * migrate to mongo2go * wrapup fixes * fix: ignore examples in coverlet * rename example

Fix/review fixes (#2)

mrnkr · 2020-12-27T15:48:30Z

Hello everyone!

Since the PR was getting too big I decided to push off the changes I mentioned before for the time being, this is going to be a nightmare to review as it stands...

Basically this is working fine with the exceptions of:

filters with variables like /filterableResources?filter=equals(someNullableInt32,someInt32) which MongoDb.Driver isn't able to translate for some reason
resource metadata is just not being used
Retrieves_all_properties_when_fieldset_contains_readonly_attribute needs to be fixed
relations

Hopefully we can still get this merged and I'll work on these fixes in future PRs, hopefully making it easier to review each of them individually!

Let me know if there is anything that needs fixing!

bart-degreed · 2020-12-28T14:38:19Z

Hi @mrnkr, you've obviously put a lot of time and effort into all of this, thanks for that!

I've skimmed through the changes in your master branch. What I noticed is that lots of tests are commented out because relationships are not supported. Do you have any thoughts on whether it is feasible to add support for that?

I'm happy to see you're working on this and I have been thinking about how to move forward with getting your changes integrated. I believe every commit should improve the codebase, so I'd prefer to not merge lots of commented-out code that's going to be fixed at a later time. We all have lives and shifting priorities, so the 'later' may never come, leaving the project in a somewhat broken state.

Therefore I'd like to propose you submit the following PRs, which I can review and take, one at a time:

Update to the latest version of JADNC with .NET Core 3.1. This would include your MongoDB repository and basic getting-started instructions, but without the example/getting-started/test projects. Sort of like: we just updated the codebase to make it run, no guarantees about what actually works.
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.
Add some form of relationships support, depending on what is possible. This would replace the tests from PR 2 that produce an error with actual working code.

Something else: I think it would be easier for you to work in separate branches on your fork and keep your master branch in sync with upstream. That way, you can easily merge changes from master into your feature branches.

mrnkr · 2020-12-28T15:26:38Z

@bart-degreed I agree with everything you said, I'll close this PR and do those other smaller ones.

In terms of relationships, I'm pretty sure it is possible but yesterday I did try looking into it and found that it was very hard, at least for me. Will not be trivial and maybe it won't be a very performant implementation since I don't see a way to include referenced documents in a query in one call, it would be a O(n) operation, and an async one at that, so it does not look like something I'd want to use in production...

I agree about the commented code issue, I did think of those as a reminder to tackle them later but it is true later may never come even though I hope it will.

mrnkr added 12 commits October 5, 2020 18:29

update dotnet version to 3.1 and reimplement

6fc9b15

feature: tests for CountAsync, CreateAsync and DeleteAsync

a6e06e6

refactor: use name of class as collection name

072d2ae

Added tests for FlushFromCache and GetAsync, TODO: create mock Resour…

b1f832b

…ceGraph that works

tested UpdateRelationshipsAsync and base case for UpdateAsync

e39f4e5

configured code coverage

b678ce0

configured travis

76a47e9

fix: add mongodb to travis

0c0fa56

test: UpdateAsync

282cbcf

refactor: migrate to xunit

a04c4c4

feature: working demo

a158d45

reorganize repo

3b14b4c

mrnkr added 5 commits November 7, 2020 17:31

fix: tests

48a8602

fix: remove unused code

b245507

refactor: remove unnecessary curly brackets

594c407

prepare first release

3b37d8b

Merge pull request #1 from mrnkr/dev

8d28fd4

first release

alastairtree mentioned this pull request Dec 14, 2020

Is this library proper to use for MongoDB? json-api-dotnet/JsonApiDotNetCore#635

Closed

maurei self-assigned this Dec 14, 2020

bart-degreed suggested changes Dec 14, 2020

View reviewed changes

mrnkr added 2 commits December 27, 2020 12:39

version bump

dfd167d

Merge pull request #3 from mrnkr/dev

a416207

Fix/review fixes (#2)

mrnkr closed this Dec 28, 2020

This was referenced Dec 29, 2020

feature: update repo #3

Merged

Add example projects and tests #4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare for Beta Release #2

Prepare for Beta Release #2

mrnkr commented Nov 2, 2020

codecov-io commented Nov 2, 2020 •

edited

Loading

alastairtree commented Dec 12, 2020

bart-degreed commented Dec 14, 2020

maurei commented Dec 14, 2020

bart-degreed left a comment •

edited

Loading

bart-degreed Dec 14, 2020

bart-degreed Dec 14, 2020

bart-degreed Dec 14, 2020

bart-degreed Dec 14, 2020

bart-degreed Dec 14, 2020

mrnkr Dec 18, 2020

bart-degreed Dec 18, 2020

bart-degreed Dec 14, 2020

mrnkr Dec 18, 2020

bart-degreed Dec 14, 2020

bart-degreed Dec 14, 2020

bart-degreed Dec 14, 2020

bart-degreed Dec 14, 2020

mrnkr commented Dec 14, 2020

alastairtree commented Dec 14, 2020

mrnkr commented Dec 18, 2020

mrnkr commented Dec 24, 2020

alastairtree commented Dec 24, 2020

mrnkr commented Dec 27, 2020

bart-degreed commented Dec 28, 2020

mrnkr commented Dec 28, 2020

		# Created by https://www.toptal.com/developers/gitignore/api/dotnetcore,vscode,macos,linux,windows
		# Edit at https://www.toptal.com/developers/gitignore?templates=dotnetcore,vscode,macos,linux,windows

Prepare for Beta Release #2

Prepare for Beta Release #2

Conversation

mrnkr commented Nov 2, 2020

codecov-io commented Nov 2, 2020 • edited Loading

Codecov Report

alastairtree commented Dec 12, 2020

bart-degreed commented Dec 14, 2020

maurei commented Dec 14, 2020

bart-degreed left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrnkr commented Dec 14, 2020

alastairtree commented Dec 14, 2020

mrnkr commented Dec 18, 2020

mrnkr commented Dec 24, 2020

alastairtree commented Dec 24, 2020

mrnkr commented Dec 27, 2020

bart-degreed commented Dec 28, 2020

mrnkr commented Dec 28, 2020

codecov-io commented Nov 2, 2020 •

edited

Loading

bart-degreed left a comment •

edited

Loading