refactor(repository-tests): move relation tests to repository-tests package

[agnes512]

resolves #3442

• Move these tests to repository-tests package so that they would be tested against real databases such as MongoDB and MySQL.
• Introduce a new type mixedType for different id types usage
• Refactor Repository classes so that they are inherited from CrudRepositoryCtor.

relation.factory.integration.ts is split into has-many.factory.integration.ts and belongs-to.factory.integration.ts

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[agnes512]

@raymondfeng I don't know if we need to export these tests as functions like what we did in this file.
We keep them as they are because they run fine.
^ nvm we are gonna change it

[bajtos]

Thank you for taking this non-trivial task!

Based on what I see in the code, I think there are few tricky areas that may require a bit of research:

• How to define model classes to take into account features.idType
• How to work with repository classes and relation repository factories/accessors.

I feel that moving all tests in one pull request is not very practical, as there is too much code involved. I am proposing to split this effort into smaller chunks that will make it easier to iterate on the test design.

Personally, I would start with a very small piece and pick a single relation type (BelongsTo or HasMany) and a single test type (e.g. integration tests for the repository factory).

[bajtos]

I find it suspicious that we need to add these new dependencies. I quickly skimmed through the code and don't see any place using them. Can you double check please?

Ideally, there should be no changes in packages/repository-tests/package.json and packages/repository-tests/package-lock.json needed.

[nabdelgadir]

We used '@loopback/context' in our repository fixtures: import {Getter, inject} from '@loopback/context';, but I don't think we use @loopback/core anywhere so I'll remove it

[bajtos]

This isn't going to work.

Your CustomerRepository is always extending DefaultCrudRepository, ignoring repositoryClass constructor provided from outside.

When you call new repositoryClass, an instance of repositoryClass (typically DefaultCrudRepository) is created. The constructor of CustomerRepository is not called. As a result, the repository instance won't have any orders property as defined in CustomerRepository.

I see two options how to move forward:

(1)
Move the code creating repositories into a factory function, accepting repositoryClass as an argument.

For example:

export function createCustomerRepository(repositoryClass: /*insert type*/) {
  return class CustomerRepository extends repositoryClass<Customer, /*how do we specify id type here? */> {
    // ...
  }
}

(2)
Don't create model-specific repository classes, do everything from outside in the test.

async function givenBoundCrudRepositories(db: juggler.DataSource) {
  orderRepo = new repositoryClass(Order, db);
  // option 1: create variable instead of a property
  orderCustomer = createBelongsToAccessor(
    /* lookup relation definition */, 
    async () => customerRepo,
    orderRepo,
  );

  // option 2: define a property - this requires `orderRepo` to be cast to something like 
  // `repositoryClass & {shipment: BelongsToAccessor<Shipment, typeof Order.prototype.id>;}`
  orderRepo.shipment = createBelongsToAccessor(
    /* lookup relation definition */, 
    async () => shipmentRepo,
    orderRepo
  );

  return {orderRepo, orderCustomer};
}

[agnes512]

We were trying to pass db to the CustomerRepository in here. I am not sure if it doesn't run on mongo case we solved some mongo failures from this file before.

[nabdelgadir]

We created a helper file to get the repositories. What do you think?

[jannyHou]

Great effort! LGTM in general, left one comment. I assume the test files are copied from the old ones, so didn't review them. The CI tests are failing and seem related, please fix them before merge :)

[jannyHou]

hmm, why do we need these default values?

[agnes512]

If we don't set the default value, MySQL somehow would have undefined instead of null

[nabdelgadir]

The CI tests are failing and seem related

They were just timeout failures. They're gone now.

[jannyHou]

MySQL somehow would have undefined instead of null

This behavior sounds reasonable to me, 🤔 , why is it a problem? Some tests expect null instead of undefined? Or any API calls will be broken?

[agnes512]

If nothing is provided then loopback uses undefined but mysql uses null. And if we use null in lb4, it's not the same thing as it's in MySQL. That's why we use these default values.

[bajtos]

It's a well-know problem (feature?) that NoSQL databases like memory and MongoDB represent missing values as undefined, while SQL databases use NULL. We have features.emptyValue to use in tests to deal with that difference.

Please remove default fields from the property definitions and modify the checks to use feature.emptyValue instead of the default or undefined.

See e.g. this test for inspiration:

https://github.com/strongloop/loopback-next/blob/6ad0bb59c71ea2356cb951da786bdbff246b47e7/packages/repository-tests/src/crud/replace-by-id.suite.ts#L73-L81

[bajtos]

Nice! I see you are making good progress here, discovering subtle differences in database-specific behavior. I find certain parts of the proposed changes problematic, let's do few more iterations to improve them.

[bajtos]

Configuring connector-specific settings in the shared test suite creates unwanted coupling between the generic test suite (that should be connector-independent) and individual connectors. It makes it difficult for individual connectors to tweak these settings, because the change must be made in the shared test suite - this is even more difficult for 3rd party connectors. Let's find a better solution please.

I am proposing to introduce a new features flag allowing each connector to provide additional model settings that should be used by all models. Example usage here:

@model({
  settings: features.modelSettings,
})
class Order extends Entity {
  // ...
}

However! I believe the MongoDB connector allows users to enable strictObjectIDCoercion at dataSource-level, for all models connected to that datasource. I prefer to enable strictObjectIDCoercion that way (see MONGODB_CONFIG) instead of introducing a new CrudFeature flag.

I'd also like to better understand why is it necessary to enable strictObjectIDCoercion? What is happening under the hood?

If strictObjectIDCoercion is necessary to make relations work, then we need to make this very clear in our documentation! Ideally, we should modify lb4 datasource generator to enable that flag in every datasource that's using the MongoDB connector. Docs & CLI updates are out of scope of this pull request, we should open a new issue to keep track of those changes. Ideally, that issue should explain why is that flag necessary (what is happening under the hood).

Very loosely related: In the future, I'd like us to rework MongoDB connector so that ObjectID type is never leaked outside of the connector, so that the Models and any user-facing code is using string instead. See #3456

[agnes512]

We use strictObjectIDCoercion here cause I checked these 2 issues: Mongo fails to find user by id, Document strictObjectIDCorecion flag. And also since we were using string, I thought it might help to deal with ObjectId.
But I don't think it works as expected. It will be removed.

[bajtos]

It's a well-know problem (feature?) that NoSQL databases like memory and MongoDB represent missing values as undefined, while SQL databases use NULL. We have features.emptyValue to use in tests to deal with that difference.

Please remove default fields from the property definitions and modify the checks to use feature.emptyValue instead of the default or undefined.

See e.g. this test for inspiration:

https://github.com/strongloop/loopback-next/blob/6ad0bb59c71ea2356cb951da786bdbff246b47e7/packages/repository-tests/src/crud/replace-by-id.suite.ts#L73-L81

[bajtos]

Great progress!

The new design using create{ModelName}Repo looks much more correct & robust to me. Well done 👏

I found few more places where we need to take into account that certain types will be different depending on the connector, please take a look.

[bajtos]

The patch looks mostly good now, I have few last & hopefully minor comments to address.

All major problems have been addressed by now.

[bajtos]

Lovely 👏

🚀