feat(repository): introduce resolver and hasMany key inferrence with belongsTo decorator AND add belongsTo repository

[shimks]

1st and 2nd commit: Type resolver and keyTo inferrence

There is an issue with relation and decorators where model resolution doesn't occur properly. A short example to illustrate the problem:

class Customer {
id: number;
@hasMany(Order) orders: Order[];
}
// in another file
class Order {
@belongsTo(Customer) customerId: number;
}

When Customer is being defined, Order will be read and be attempted to be defined. As Order is being defined, it comes across Customer which has yet to be defined, so the metadata being stored by the decorators is incomplete at the time it's being stored.

The work around for this is to introduce a resolver; a resolver is a function that would return the given model when called. So instead of passing in the Models themselves these decorators, you would pass in their resolver instead: @hasMany(() => Order) and @belongsTo(() => Customer).

Another feature included in these commits is automatic building of hasMany metadata based on existing metadata on belongsTo. More specifically, keyTo is inferred from the name of the property key in which belongsTo metadata exists.

In summary, this PR introduces:

• logic to detect circular dependency (only with @hasMany and @property.array)
• keyTo inference from the key decorated by @belongsTo

3rd commit: belongsTo repository

There is a problem where if belongsTo and hasMany relations are defined on the repositories, our context system will throw an error due to the cyclic dependence the relations create. To illustrate the example in a dirty dirty code snippet:

// ... customer repo constructor
constructor(@repository(OrderRepository) orderRepo)
// ... order repo constructor
constructor(@repository(CustomerRepository) customerRepo)

Take an event where customer repository needs to be instantiated. An injection on orderRepo will attempted to be made and an order repository will be instantiated. When an order repository is being instantiated, a customer repository needs to be injected, and therefore a circular dependency is established.

The solution to solving the dependency is to use a resolver like we did for the commits before. Take a look at this code snippet and this to see how the UX would look like. I'll probably work on introducing a decorator that won't promisify the repositories here.

related to #1361

Things that have not been done:

• Create a decorator to replace @inject.getter usage in our relation repositories
• Extraction of common code between hasMany and belongsTo
• Extensive test coverage
• License headers
• Docs and TSDocs

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

[shimks]

this logic here only accounts for circular dependency through itemType property. This should also be extended to cover type as well

[shimks]

I want to change the relation decorator signature to something like this:

export function relation(
  targetOrDefinition: T | ModelResolver<T> | relationDefinitionBase,
  propertyDef?: propertyDefinition
)

Thoughts?

[virkt25]

Why would we require propertyDef?

[shimks]

Oh oops, meant the propertyDef to be optional. I've fixed the OP with the edit

[raymondfeng]

I guess we should generalize it to be TypeResolver.

[raymondfeng]

tsdocs :-).

[raymondfeng]

I'm seeing many places to check and resolve. It's be nice to have a common resolveType function.

[bajtos]

In Context, we have a Getter type. It makes me wonder if it makes sense to unify our terminology here? Maybe rename Getter to AsyncGetter and use modelGetter instead of modelResolver here?

Just an idea to consider, I am not entirely sure if it's a good one.

UPDATE
As I finishing review of the patch and re-read the new implementation based on TypeProvider, I think your current version is fine. Please ignore this comment.

[jannyHou]

According to the tests here, does it mean people couldn't import 2 related models in one controller file?

[shimks]

Actually, the person wouldn't be able to import either of the models at all since they are both related

[jannyHou]

Had a talk with @shimks , and thanks for explaining what this test case aims to catch.

Here is a summary(correct me if wrong):

This test is to assert that a proper error is thrown when users define the relation in a wrong way, like:

@model()
Customer{
  // The correct signature is using a type resolver 
  // @hasMany(()=>Order)
  @hasMany(Order)
  orders
}

[jannyHou]

Hmm I am a bit confused here: according to the test case, a hasMany relation should always come with a corresponding belongsTo relation, while shouldn't they be decoupled?

---

Update

Sorry I post the question at the wrong line, it aims to ask about the test case above it ^
"throws if no target relation metadata is found"

[shimks]

This test covers the case when a complementing belongsTo relation is defined.

[jannyHou]

And thanks again for explaining the test above!

A brief summary:

The keyTo property must be either provided in the parameter of @hasMany() decorator, or be inferred from the target model's belongsTo relation. The test fails because keyTo is missing in both places.

I feel it would be good to document this behaviour somewhere so people know the correct way to define the keyTo property :)

[shimks]

There is still more that needs to be done with this commit; more tests need to be added and i need to extract out similar code from both hasMany and belongsTo.

I mainly want to get people's opinion on the work I have done so far.

[shimks]

Due to the issue of dependency cycle, a getter needs to be used and eventually invoked to resolve the target repository. I'm thinking that we need a separate decorator as per @raymondfeng's suggestion. Something like @getRepository?

[virkt25]

I wonder if @repository can be extended to provide us that support?

[shimks]

I think it's possible. Let me tinker around with it

[shimks]

Visited the code for @repository. Here are the options that I see:

1. pass in an option along the lines of resolveCircularDependency. When enabled, @repository can call inject.getter underneath instead of inject
2. Detect if the prototype passed in to the decorator inherits from DefaultCrudRepository and then call inject.getter. This allows the UX to remain the same as before, but there might be a headache maintaining this with other future implementations of repositories. I also don't know easy it'd be to figure out whether the prototype belongs to DefaultCrudRepository or not

[virkt25]

I see. @raymondfeng @bajtos thoughts?

[bajtos]

I don't fully comprehend the issues we are facing here.

• I think the API should be explicit. It should be obvious to people reading the code whether a repository or a getter is injected.
• To follow the pattern @inject.getter, I am proposing to create a new decorator @repository.getter.

[shimks]

this is from the effect of trying to solve the circular dependency issue. As mentioned above, we could have a separate decorator so that we don't end up using a promisified version of the repository

[virkt25]

Can we not inject the repository instead using @inject.getter()? or add support for this via @repository?

[bajtos]

Please split this statement into two lines to avoid too many parenthesis and make error stack traces more helpful.

const repository = await this.targetRepositoryGetter();
return repository.create(
  // ...
);

Same comment applies to other modified repository methods below.

[virkt25]

Why is the exclude necessary? Don't think I've seen issues before.

[shimks]

The way the mocha script is run, it attempts run every JS files in the test folder. In this case, the exclude is necessary since reading the bad cyclic files (that are supposed to be read and caught in a test block) result in an error being thrown from the new code.

That being said, there might be a better approach to having mocha not read these files.

[virkt25]

I'm ok with the exclude, just asked cause I was curious why we needed it.

[virkt25]

This is only used by 2 tests, any reason we shouldn't just declare this within the test it blocks?

[shimks]

Not really. I can revert the change Nvm, apparently there are other tests that require CustomType

[virkt25]

Should the error say a user should use a resolver?

[shimks]

I believe that error messages should remain as literal as possible, but in this case it might be more reasonable to suggest users to use a resolver since the condition that checks for this code path is pretty hard to meet any other way.

I think I'll make this change

[virkt25]

it's better when an error message can tell you how to fix the problem imo :)

[virkt25]

Is this intentional? Why are we making this optional?

[shimks]

The intention behind it was to allow users to just pass in type and target so that the rest of the properties could be inferred later down the road. However keyTo is something that is required for the relation engine to work, so maybe a better solution is to have a type for that intermediate phase when keyTo hasn't been inferred yet. Thoughts?

[virkt25]

Hmm ... as long as we update the docs to say how we will attempt to infer the keyTo property I'm ok with this change. And if a lot of users have an issue we can look into other solutions.

[virkt25]

Why would we require propertyDef?

[virkt25]

delete juggler.

[virkt25]

Doesn't either keyTo or keyFrom need to be required?

[shimks]

Yup, as mentioned in #1618 (comment), I think a solution to this is to introduce a intermediate type definition that is used before the relation metadata is resolved

[virkt25]

I wonder if @repository can be extended to provide us that support?

[virkt25]

Can we not inject the repository instead using @inject.getter()? or add support for this via @repository?

[virkt25]

Change this to beforeEach to preserve state after a failed test.

[bajtos]

Yes please!

https://loopback.io/doc/en/lb4/Testing-your-application.html#clean-the-database-before-each-test

Start with a clean database before each test. This may seem counter-intuitive: why not reset the database after the test has finished? When a test fails and the database is cleaned after the test has finished, then it’s difficult to observe what was stored in the database and why the test failed. When the database is cleaned in the beginning, then any failing test will leave the database in the state that caused the test to fail.

[bajtos]

Nice work!

[bajtos]

In Context, we have a Getter type. It makes me wonder if it makes sense to unify our terminology here? Maybe rename Getter to AsyncGetter and use modelGetter instead of modelResolver here?

Just an idea to consider, I am not entirely sure if it's a good one.

UPDATE
As I finishing review of the patch and re-read the new implementation based on TypeProvider, I think your current version is fine. Please ignore this comment.

[bajtos]

IGNORE THE TEXT BELOW, I HAVE CROSS-POSTED THIS COMMENT AS #1618 (comment)

---

Hmm, this is not very reliable, is it?

Once we add support for JavaScript, people will be able to write models using the pre-ES6 way where no class keyword is used.

function MyModel(data) {
  Entity.call(this, data);
  // the rest of the constructor
}

util.inherits(MyModel, Entity);

If a model/entity class has to always inherit from our Model/Entity, then it may be more robust to check fn.prototype inheritance chain, something along the lines

return !(fn.prototype instanceof Model)

This assumes the Model constructor was loaded by the application from the same instance of @loopback/repository as our isModelResolver function was loaded from, which may not be always the case.

A more robust option is to crawl inheritance chain and check if a constructor name matches Model.

for (proto = fn.prototype; proto != null, proto = Object.getPrototypeOf(proto)) {
  if (proto.constructor.name === 'Model') return true;
}
return false;

To me, this complexity is a signal that we need to look for a different API design, one where we don't need to distinguish between a model constructor function and a model provider function.

[bajtos]

Please use more descriptive names than X and Y. For example, you can use Customer and Order that we are already using elsewhere. Since you are modeling N-N relation, Product and Category may be better model names (a product can belong to multiple categories and each category can contain multiple products).

[bajtos]

The test names does not read like a sentence to me, did you perhaps mean "it errors when keyTo is not available in hasMany metadata"?

[bajtos]

I don't fully comprehend the issues we are facing here.

• I think the API should be explicit. It should be obvious to people reading the code whether a repository or a getter is injected.
• To follow the pattern @inject.getter, I am proposing to create a new decorator @repository.getter.

[bajtos]

I think getTargetRepository would be a better name?

[bajtos]

I think getTargetRepository would be a better name?

[bajtos]

What is the point of this repository class? I mean BelongsToFactory is returning Promise<Target> therefore we are not going to use this repository implementation in applications (controllers).

If we decide to keep this class, then please:

• mark it as implements BelongsToRepository<...>
• modify BelongsToFactory to return BelongsToRepository<...> instead of Promise<Target>.

(I personally think that returning Promise<Target> is pretty fine because there isn't much one can do with the model we are belonging to.)

[shimks]

I thought we could keep the repository pattern for potential addition of methods that a belongsTo repository could have. An example would be a method that can change what model the given model could belong to.

[bajtos]

I thought we could keep the repository pattern for potential addition of methods that a belongsTo repository could have. An example would be a method that can change what model the given model could belong to.

Makes sense. I agree that a repository is easier to extend than the current Promise<Target> API.

In that case, let's follow the plan B outlined in my comment above:

• mark it as implements BelongsToRepository<...>
• modify BelongsToFactory to return BelongsToRepository<...> instead of Promise<Target>.

Thoughts?

I hope I am not missing something important here.

[shimks]

If we were to implement your suggestion, the controller method for finding the related model would look something like this:

findCustomer(id: number) {
  return await orderRepo.customer(id).find();
  // instead of: await orderRepo.customer(id);
}

I know that @raymondfeng and @virkt25 like BelongsToFactory returning Promise<Target> here, but I can also see that we probably don't want the API changing too drastically in the future.
Thoughts?

[bajtos]

I am personally find with both flavors. Both of them have pros and cons, I think we need to make a trade-off here.

• orderRepo.customer(orderId)
• orderRepo.customer(orderId).find().

Can we rename the repository method from find to get please?

findCustomer(id: number) {
  return await orderRepo.customer(id).get();
}

[shimks]

I've decided to go with the orderRepo.customer(orderId), primarily because it requires the least amount of deviation from our currently established pattern with DefaultHasManyEntityCrudRepository where we pass in a 'constraint' to the relation repository.

[bajtos]

Fair enough 👍

[bajtos]

Yes please!

https://loopback.io/doc/en/lb4/Testing-your-application.html#clean-the-database-before-each-test

Start with a clean database before each test. This may seem counter-intuitive: why not reset the database after the test has finished? When a test fails and the database is cleaned after the test has finished, then it’s difficult to observe what was stored in the database and why the test failed. When the database is cleaned in the beginning, then any failing test will leave the database in the state that caused the test to fail.

[bajtos]

See my earlier comment.

This implementation requires two database calls.

Ideally, findOwnerOfOrder should make a single database query only.

# a mock up, may not work
SELECT * 
  FROM Customer 
  JOIN Order ON Order.customerId = Customer.id
  WHERE Order.id = ${orderId}

Even if our implementation cannot do such SQL JOIN query yet, the API should be designed in a way that will allow such implementation in the future.

For example:

// a mock-up implementation of belongsTo "find" function
// assuming we can query related properties
const data = await this.customerRepository.find({
  include: ['orders'],
  where: {'order.id': orderId},
  fields: {orders: true}
});
return data.orders[0];

[bajtos]

Cross-posting #1618 (comment).

Hmm, this is not very reliable, is it?

Once we add support for JavaScript, people will be able to write models using the pre-ES6 way where no class keyword is used.

function MyModel(data) {
  Entity.call(this, data);
  // the rest of the constructor
}

util.inherits(MyModel, Entity);

If a model/entity class has to always inherit from our Model/Entity, then it may be more robust to check fn.prototype inheritance chain, something along the lines

return !(fn.prototype instanceof Model)

This assumes the Model constructor was loaded by the application from the same instance of @loopback/repository as our isModelResolver function was loaded from, which may not be always the case.

A more robust option is to crawl inheritance chain and check if a constructor name matches Model.

for (proto = fn.prototype; proto != null, proto = Object.getPrototypeOf(proto)) {
  if (proto.constructor.name === 'Model') return true;
}
return false;

To me, this complexity is a signal that we need to look for a different API design, one where we don't need to distinguish between a model constructor function and a model provider function.

---

❗️ By making this type more generic, i.e. a provider for any type, it becomes even more difficult to distinguish between a type and a provider. For example, what if the type is a function? Or a built-in type like Date, RegExp or Buffer?

$ node
> RegExp.toString()
'function RegExp() { [native code] }'
> Date.toString()
'function Date() { [native code] }'
> Buffer.toString()
'function Buffer(arg, encodingOrOffset, length) {\n  showFlaggedDeprecation();\n  // Common case.\n  if (typeof arg === \'number\') {\n    if (typeof encodingOrOffset === \'string\') {\n      throw new ERR_INVALID_ARG_TYPE(\'string\', \'string\', arg);\n    }\n    return Buffer.alloc(arg);\n  }\n  return Buffer.from(arg, encodingOrOffset, length);\n}'

[shimks]

I'm personally fine with the complex proposal you've given here, mainly because I can't think of a better alternative API design that wouldn't need to differentiate between a constructor and a function returning a constructor. In regards to addressing built-in types like Date (which I don't really want to do in this PR so that I can land it faster), I think that can be hard coded to check for, and addressing plain functions might be doable with another complex and hacky check.

Do you have an alternative in mind? I've been mulling over a way to support the relation decorators taking in either the resolver or the model constructor, but I haven't come up with a clean alternative.

[bajtos]

My personal conclusion is that we need to modify the design of relation decorators so that we don't need to distinguish between a type (a model class) and a type provider (a model class provider). This is one of the reason why our Binding class has so many different methods for setting the value (to(), toDynamicValue(), toClass and toProvider) - JavaScript is not powerful enough to allow code to differentiate between these different types at runtime.

[shimks]

There are two things I have yet to address with the latest commit:

1. factory parameter change from model instance to model id
2. beefing up TypeResolver check.

[bajtos]

I have reviewed the new commit, it goes in the right direction 👍

[bajtos]

@repository support multiple flavors:

• repository(repositoryName: string | Class<Repository<Model>>)
• repository(model: string | typeof Entity, dataSource: string | juggler.DataSource)

I am ok to support only bindingKey flavor (i.e. repository(repositoryName: string)) in this pull request, we can add support for other flavors later.

However, I am wondering whether we should add some checks to throw a helpful error if this new decorator is injected in a way that's not supported? Unless the function signature is good enough to allow the TypeScript compiler to reject such programmer mistakes.

Few examples of incorrect usage that I am thinking of:

• @repository.getter(ProductRepository)
• @repository.getter('Product', dbDataSource)
• @repository.getter(Product, 'db')

[shimks]

I believe all of the incorrect usage should be covered BindingAddress. It can be strengthened so that T can extend from EntityCrudRepository though.

[bajtos]

I believe all of the incorrect usage should be covered BindingAddress.

Sounds good.

[shimks]

I'm hoping this is the last iteration of reviews I'd have to go through.

One thing to note: I wanted to create a _createRelationFactoryFor function which would combine the functionality of creating a constrained repository of both hasMany and belongsTo repository, but when I've tried an implementation the factory function required a hard casting of the repository being returned from the user. I thought this was counter intuitive so I ditched that effort.

[bajtos]

One thing to note: I wanted to create a _createRelationFactoryFor function which would combine the functionality of creating a constrained repository of both hasMany and belongsTo repository, but when I've tried an implementation the factory function required a hard casting of the repository being returned from the user. I thought this was counter intuitive so I ditched that effort.

+1 for finding a different solution than a factory function requiring an explicit type cast.

[bajtos]

My main remaining concern is about isTypeProvider and places accepting dual Type | TypeProvider values. I feel this is a cornerstone of our design and I fear that changing this later would be difficult, especially when considering upgrade costs for our users. Let's think a bit more about different ways how to address this problem before we settle on a solution.

The other comments are mostly cosmetic.

[bajtos]

A TODO comment to address?

[bajtos]

A TODO comment to address?

☝️

[bajtos]

Can we exclude all DIST/test/fixtures files please, as we do in the top-level package.json file?

[bajtos]

AFAICT, we are using type-resolver version in @hasMany and @belongsTo decorators in the existing examples.
IMO, we should apply the same change here and drop support for target: typeof Entity. I hope such change will allow us to get rid of fragile isTypeResolver check.

Thoughts?

[bajtos]

Ditto. Let's change the first argument to targetModelResolver: TypeResolver<T>

[bajtos]

Ditto. Let's change the first argument to targetModelResolver: TypeResolver<T>

[bajtos]

the related target model repository instance is misleading, please fix the comment to mention a getter.

[bajtos]

Ditto. the related target model repository instance is misleading, please fix the comment to mention a getter.

[bajtos]

Please remove "Repository" from the name of this method, since it does not return any repository.

Proposed name: _createBelongsToAccessorFor or _createBelongsToFactoryFor.

While we are discussing these names. I find createBelongsToFactory name as misleading, because this helper function is not returning a factory (a function creating something new), it returns a function for accessing the related model instance (an accessor, a getter).

Could you please take another look and see if you can come with names that better communicate the intent of these two functions/methods?

[bajtos]

If you rename createBelongsToFactory per my comment above, then please rename BelongsToFactory type accordingly too.

[bajtos]

As I commented above (see #1618 (comment) and #1618 (comment)), I'd prefer to design these changes in such way that we don't need isTypeResolver at all, because I am concerned about edge cases where it may not be possible to automatically distinguish between a type and a type resolver, especially when classes and function values in general get involved.

How about using a different property to store the type resolver value? The idea is that PropertyDefinition should provide either a type: PropertyType or a typeResolver: TypeResolver.

export interface PropertyTypeValue {
  type: PropertyType;
}

export interface PropertyResolvedType {
  typeResolver: TypeResolver;
}

export interface PropertyArrayValue {
  type: 'array';
  itemType: PropertyType;
}

export interface PropertyResolvedArray {
  type: 'array';
  itemTypeResolver: PropertyTypeResolver;
}

export interface PropertyTypeDefinition =
 | PropertyTypeValue
 | PropertyResolvedType
 | PropertyArrayValue
 | PropertyResolvedArray;

export interface PropertyDefinition =  PropertyTypeDefinition & {
  id?: boolean;
  // etc.
}

This can be accompanied by property decorators accepting a type resolver, for example @resolvedProperty and @resolvedProperty.array.

Thoughts?

cc @raymondfeng @strongloop/sq-lb-apex

[shimks]

An alternative I've also thought of is to introduce a property called isTypeResolver, which would be set when the relation decorators and the example decorators you've mentioned above. The pro would be that we wouldn't need to introduce different property type definitions while the con would be that we may need to check whether it's type that's a resolver or itemType. Thoughts?

[raymondfeng]

I see pros and cons on both sides. If we can reliably detect the resolverFunction, I will vote for reusing the same names.

To generalize the idea, we should try to allow OpenAPI like schema definitions and introduce a separate decorator such as @schema to decouple the typing from @property?

@property({name: 'my-prop'})
@schema({type: ...})
myProp: string;

The @schema should allow one of the following:

1. {type: 'number'}
2. {$ref: '#/components/schemas/Customer'}
3. {type: Customer}
4. {type: () => Customer}. (Maybe {$ref: () => Customer} is even better)

[shimks]

I'm thinking that we don't need @schema as long as we have one of the two changes me and miroslav had mentioned implemented.

The only places where a model and its resolver would need to be differentiated is in the JSON schema conversion logic (where a resolver would have to be resolved first) and in keyTo inference for resolveHasManyMetadata/resolveBelongsToMetadata, where the latter option may not even be a concern since we're heading towards the direction of dropping the use case for @hasMany(AModelConstructor).

For the sake of landing this PR, it might be better to stay with the current coupling of providing type information and the juggler.

This reminds me, with the proposed change in this comment thread, do you think that the use case for the relation decorators taking in the model constructor can stay?

[raymondfeng]

The code can be simplified as follows:

 definition = definition || {};
 const isCyclic = definition.hasOwnProperty('type') && !definition.type;
 const isCyclicArray =
    (definition.type === Array || definition.type === 'array') &&
    definition.hasOwnProperty('itemType') &&
    !definition.itemType;

[raymondfeng]

We should refactor it to a utility function, such as isTypeDefined(definition, key).

[raymondfeng]

So we don't allow @belongsTo(User) any more? Does it have to be @belongsTo(() => User)?

[shimks]

Yes, this would be the approach we'd need to take if we want to minimize the cases where we need to differentiate between User and () => User

[bajtos]

Let's keep this discussion at the top-level please, so that it's not collapsed/hidden if we change this particular line of code.

See my comment here: #1618 (comment)

[b-admike]

Should we add checks for orderSchema.definitions and calling expectValidJsonSchema on customerSchema for this test?

[b-admike]

nitpick: can we reword this to something like: Please consider using TypeResolver as such: (() => TargetModel)

[bajtos]

@raymondfeng

So we don't allow @belongsTo(User) any more? Does it have to be @belongsTo(() => User)?

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.

Related comments:

#1618 (comment)

• I think the API should be explicit. It should be obvious to people reading the code whether a repository or a getter is injected.
• To follow the pattern @inject.getter, I am proposing to create a new decorator @repository.getter.

#1618 (comment)
I am proposing to simplify our implementation, always require Getter<EntityCrudRepository<Target, TargetID>> and drop support for targetRepo: EntityCrudRepository<Target, TargetID>.

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.

#1618 (comment)

The discussion revolves about the ways how to distinguish between a class and a getter function, which is needed to distinguish between @belongsTo(User) and @belongsTo(() => User). Check it out for examples if difficult edge cases. The conclusion:

My personal conclusion is that we need to modify the design of relation decorators so that we don't need to distinguish between a type (a model class) and a type provider (a model class provider). This is one of the reason why our Binding class has so many different methods for setting the value (to(), toDynamicValue(), toClass and toProvider) - JavaScript is not powerful enough to allow code to differentiate between these different types at runtime.

#1618 (comment)

AFAICT, we are using type-resolver version in @hasMany and @belongsTo decorators in the existing examples.
IMO, we should apply the same change here and drop support for target: typeof Entity. I hope such change will allow us to get rid of fragile isTypeResolver check.

#1618 (comment)

Here we discuss PropertyDefinition interface and whether type should accept both PropertyType | TypeResolver. The discussion is not resolved AFAICT, I am not sure if it is or is not relevant to what type is accepted by@belongsTo (model class vs. resolver).

[bajtos]

I am going to split this huge patch into multiple smaller and more-manageable chunks. The first PR adding TypeProvider to property definitions: #1751

[bajtos]

Next steps:

• rework @hasMany and other relation decorators to accept TypeProvider only, e.g.@hasMany(() => Product) - see Break cyclic dependencies in relations #1787
• automatic building of hasMany metadata based on existing metadata on belongsTo. More specifically, keyTo is inferred from the name of the property key in which belongsTo metadata exists.
• belongsTo repository - see feat(repository): implement belongsTo relation #1794
• check for cyclic dependencies in @property()

[bajtos]

check for cyclic dependencies in @Property()

I have created a follow-up issue to address this in a more thorough way - see #1799.

automatic building of hasMany metadata based on existing metadata on belongsTo. More specifically, keyTo is inferred from the name of the property key in which belongsTo metadata exists.

Created a follow-up issue - see #1800

[bajtos]

Closing as done via #1751, #1787 and #1794.