Break cyclic dependencies in relations

[bajtos]

Rework HasMany-related code to avoid cyclic dependencies after introducing belongsTo relation. This pull request is another step on my journey to land #1618 in several smaller chunks.

Done:

• Modify @hasMany decorator to always accept TypeResolver for the target model (instead of the target model class as before).

• Modify HasManyRepository implementation and factory to accept the target model via a Getter (instead of a model class as before).

• Update repository-json-schema to understand TypeResolver properties.

• Review the code coverage report and add missing tests

See the comment below for detailed reasoning why these change are made in this particular way.

This pull request is related to #1361.

Checklist

• 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

[bajtos]

Modify @hasMany decorator to always accept TypeResolver for the target model (instead of the target model class as before).

Before this change, a relation property was decorated as follows:

@model()
class Customer extends Entity { 
  // ...

  @hasMany(Order)
  orders: Order[];
}

My pull request is changing this API as follows:

@model()
class Customer extends Entity { 
  // ...

  @hasMany(() => Order)
  orders: Order[];
}

I am cross-posting #1618 (comment) below to explain why I am forcing users to always use TypeProvider even in cases where a model constructor would work too.

My argument: While @belongsTo(User) may look like a simpler solution, it also means users will have to rework their codebase as soon as they introduce a cyclic relation (e.g. add @hasMany(Order).

IMO, most relations are bidirectional and come in a pair (User has many Orders, thus Order belongs to User. Category has many and belongs to many Products, thus Product has many and belongs to many Categories).

I personally prefer to start with the more flexible solution from the beginning, instead of allowing the users to pick a simpler path that will backfire on them in the next step.

---

Modify HasManyRepository implementation and factory to accept the target model via a Getter (instead of a model class as before).

This is a similar change.

Before:

class CustomerRepository extends DefaultCrudRepository<
   Customer,
   typeof Customer.prototype.id
 > {
   public orders: HasManyRepositoryFactory<Order, typeof Customer.prototype.id>;
   constructor(
     @inject('datasources.db') protected db: juggler.DataSource,
     @repository(OrderRepository) orderRepository: OrderRepository,
   ) {
     super(Customer, db);
     this.orders = this._createHasManyRepositoryFactoryFor(
       'orders',
       orderRepository,
     );
   }

After:

class CustomerRepository extends DefaultCrudRepository<
   Customer,
   typeof Customer.prototype.id
 > {
   public orders: HasManyRepositoryFactory<Order, typeof Customer.prototype.id>;
   constructor(
     @inject('datasources.db') protected db: juggler.DataSource,
     @repository.getter(OrderRepository)
     getOrderRepository: Getter<OrderRepository>,
   ) {
     super(Customer, db);
     this.orders = this._createHasManyRepositoryFactoryFor(
       'orders',
       getOrderRepository,
     );
   }

My reasoning is similar too, see #1618 (comment):

That way people don't have to upgrade their code from EntityCrudRepository to Getter</*...*/> at the point when they add a new relation introducing cyclic dependency.

[b-admike]

I think there are still some todo items left in this PR, so I look forward to review it again, but it looks great so far! I've left some questions/concerns.

[b-admike]

I'm curious; why do we return the model definition here? wouldn't the first line suffice?

[bajtos]

See https://en.wikipedia.org/wiki/Fluent_interface

A fluent interface is normally implemented by using method chaining to implement method cascading (in languages that do not natively support cascading), concretely by having each method return this (self).

[b-admike]

I might be mistaken, but isn't this check similar to the one below?

[bajtos]

Good catch 👍

[b-admike]

Can you clarify what StaticMembers = Function does? (sorry I havent seen it before)

[bajtos]

Let's say we have the following resolver:

const resolver: TypeResolver<Entity> = () => Customer;
const type = resolver();

Before my change, type was set to Class<Entity>, which is defined as a constructor function with arbitrary static properties.

export interface Class<T> {
  // new MyClass(...args) ==> T
  new (...args: any[]): T;
  // Other static properties/operations
  [property: string]: any;
}

These arbitrary static properties are effectively disabling compiler type checks and preventing VSCode from showing hints for static properties.

Consider the following snippet:

type.modelName

When type is just Class<Entity>, modelName is treated as an arbitrary indexer property of type any.

This pull request is making two changes:

1. Adding a StaticMembers template argument allowing users of TypeProvider to specify what static properties are expected on the resolved type. For example:

   const resolver: TypeResolver<Entity, typeof Entity> = () => Customer;
   const type = resolver();
   type.modelName 
   // ^-- TypeScript knows this is a `string` property
   // and VSCode shows a hint

2. Providing a default value for the new StaticMembers template argument. We use Function as the default value, because Function is the common denominator of all types (every class is a Function). As a result, the value returned by a type provider has only few static properties defined, e.g. name: string.

@b-admike Does it make sense now? I am going to capture this information in a code comment for future readers.

[bajtos]

What is also important: before my change, you could treat modelName as a function, write model.modelName(), the compiler will not complain and you will discover the mistake later at runtime. With my change in place, the compiler is able to type-check known static properties and point out that modelName is a string that cannot be invoked like a function.

[b-admike]

Is the reason we don't have keyTo here because the class resolution hasn't happened yet or we don't have use for it at this point?

[bajtos]

With the current codebase, we could infer keyTo inside the @hasMany decorator and set it in relation metadata. However, one of the next steps is to change the way how the foreign key is inferred and leverage @belongsTo decorator (see https://github.com/strongloop/loopback-next/pull/1618/files#diff-444ac7611514971eb1c8e5790e78035cR135). Inspecting @belongsTo properties on the target model is the part that cannot be done from @hasMany but must wait until all classes are created and the target type can be resolved.

Setting aside @belongsTo, I think it's still a good idea keep keyTo optional, because it simplifies relation configuration for people not using decorators but writing the model definition manually via ModelDefinition class.

[bajtos]

I have addressed the comments and added more tests in 537644e. I am going to proceed with landing this PR as-is, I am happy to address any further feedback in follow-up PRs.