Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(repository): hasOne relation #1879

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
// Copyright IBM Corp. 2017,2018. All Rights Reserved.
// Node module: @loopback/example-todo
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import * as debugFactory from 'debug';
import {camelCase} from 'lodash';
import {DataObject} from '../../common-types';
import {InvalidRelationError} from '../../errors';
import {Entity} from '../../model';
import {EntityCrudRepository} from '../../repositories/repository';
import {isTypeResolver} from '../../type-resolver';
import {Getter, HasOneDefinition} from '../relation.types';
import {DefaultHasOneRepository, HasOneRepository} from './has-one.repository';

const debug = debugFactory('loopback:repository:has-many-repository-factory');

export type HasOneRepositoryFactory<Target extends Entity, ForeignKeyType> = (
fkValue: ForeignKeyType,
) => HasOneRepository<Target>;

/**
* Enforces a constraint on a repository based on a relationship contract
* between models. For example, if a Customer model is related to an Address model
* via a HasOne relation, then, the relational repository returned by the
* factory function would be constrained by a Customer model instance's id(s).
*
* @param relationMeta The relation metadata used to describe the
* relationship and determine how to apply the constraint.
* @param targetRepo The repository which represents the target model of a
* relation attached to a datasource.
* @returns The factory function which accepts a foreign key value to constrain
* the given target repository
*/
export function createHasOneRepositoryFactory<
Target extends Entity,
TargetID,
ForeignKeyType
>(
relationMetadata: HasOneDefinition,
targetRepositoryGetter: Getter<EntityCrudRepository<Target, TargetID>>,
): HasOneRepositoryFactory<Target, ForeignKeyType> {
const meta = resolveHasOneMetadata(relationMetadata);
debug('Resolved HasOne relation metadata: %o', meta);
return function(fkValue: ForeignKeyType) {
// tslint:disable-next-line:no-any
const constraint: any = {[meta.keyTo]: fkValue};
return new DefaultHasOneRepository<
Target,
TargetID,
EntityCrudRepository<Target, TargetID>
>(targetRepositoryGetter, constraint as DataObject<Target>);
};
}

type HasOneResolvedDefinition = HasOneDefinition & {keyTo: string};

/**
* Resolves given hasMany metadata if target is specified to be a resolver.
* Mainly used to infer what the `keyTo` property should be from the target's
* belongsTo metadata
* @param relationMeta hasMany metadata to resolve
*/
function resolveHasOneMetadata(
relationMeta: HasOneDefinition,
): HasOneResolvedDefinition {
if (!isTypeResolver(relationMeta.target)) {
const reason = 'target must be a type resolver';
throw new InvalidRelationError(reason, relationMeta);
}

if (relationMeta.keyTo) {
// The explict cast is needed because of a limitation of type inference
return relationMeta as HasOneResolvedDefinition;
}

const sourceModel = relationMeta.source;
if (!sourceModel || !sourceModel.modelName) {
const reason = 'source model must be defined';
Copy link
Contributor

@jannyHou jannyHou Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok to define reason twice in one function?
The first one is on https://github.com/strongloop/loopback-next/pull/1879/files#diff-a42257687a98a97022f7eb63398d2269R68

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's block-scoped.

throw new InvalidRelationError(reason, relationMeta);
}

const targetModel = relationMeta.target();
debug(
'Resolved model %s from given metadata: %o',
targetModel.modelName,
targetModel,
);
const defaultFkName = camelCase(sourceModel.modelName + '_id');
const hasDefaultFkProperty =
targetModel.definition &&
targetModel.definition.properties &&
targetModel.definition.properties[defaultFkName];

if (!hasDefaultFkProperty) {
const reason = `target model ${
targetModel.name
} is missing definition of foreign key ${defaultFkName}`;
throw new InvalidRelationError(reason, relationMeta);
}

if (
!targetModel.definition.properties[defaultFkName].id === true &&
!targetModel.definition.properties[defaultFkName].generated === false
) {
// throw InvalidRelationError('property must be a generated id field')
}

return Object.assign(relationMeta, {keyTo: defaultFkName});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we allow override the default fk in the relationMeta? This line implies we don't?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I believe we should allow users to provide their own keyTo value. @b-admike please add a test to ensure this use-case is supported.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually: this line is executed only when relationMeta.keyTo was falsey, see lines 72-75 above.

  if (relationMeta.keyTo) {
    // The explict cast is needed because of a limitation of type inference
    return relationMeta as HasOneResolvedDefinition;
  }

}
34 changes: 28 additions & 6 deletions packages/repository/src/relations/has-one/has-one.decorator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,37 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {Entity, EntityResolver} from '../../model';
import {relation} from '../relation.decorator';
import {RelationType} from '../relation.types';
import {HasOneDefinition, RelationType} from '../relation.types';

/**
/*
* Decorator for hasOne
* @param definition
* infers foreign key name from target model name unless explicitly specified
* @param targetResolver Target model for hasOne relation
* @param definition Optional metadata for setting up hasOne relation
* @returns {(target:any, key:string)}
*/
export function hasOne(definition?: Object) {
const rel = Object.assign({type: RelationType.hasOne}, definition);
return relation(rel);
export function hasOne<T extends Entity>(
targetResolver: EntityResolver<T>,
definition?: Partial<HasOneDefinition>,
) {
return function(decoratedTarget: Object, key: string) {
// property.array(targetResolver)(decoratedTarget, key);

const meta: HasOneDefinition = Object.assign(
// default values, can be customized by the caller
{},
// properties provided by the caller
definition,
// properties enforced by the decorator
{
type: RelationType.hasOne,
name: key,
source: decoratedTarget.constructor,
target: targetResolver,
},
);
relation(meta)(decoratedTarget, key);
};
}
93 changes: 93 additions & 0 deletions packages/repository/src/relations/has-one/has-one.repository.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// Copyright IBM Corp. 2017,2018. All Rights Reserved.
// Node module: @loopback/example-todo
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {Getter} from '@loopback/context';
import {DataObject, Options} from '../../common-types';
import {Entity} from '../../model';
import {Filter, Where} from '../../query';
import {
constrainDataObject,
constrainFilter,
} from '../../repositories/constraint-utils';
import {EntityCrudRepository} from '../../repositories/repository';

/**
* CRUD operations for a target repository of a HasMany relation
*/
export interface HasOneRepository<Target extends Entity> {
/**
* Create a target model instance
* @param targetModelData The target model data
* @param options Options for the operation
* @returns A promise which resolves to the newly created target model instance
*/
create(
targetModelData: DataObject<Target>,
options?: Options,
): Promise<Target>;

/**
* Find the only target model instance that belongs to the declaring model.
* @param filter Query filter without a Where condition
* @param options Options for the operations
* @returns A promise of the target object or null if not found.
*/
get(
filter?: Exclude<Filter<Target>, Where<Target>>,
options?: Options,
): Promise<Target | undefined>;
}

export class DefaultHasOneRepository<
TargetEntity extends Entity,
TargetID,
TargetRepository extends EntityCrudRepository<TargetEntity, TargetID>
> implements HasOneRepository<TargetEntity> {
/**
* Constructor of DefaultHasManyEntityCrudRepository
* @param getTargetRepository the getter of the related target model repository instance
* @param constraint the key value pair representing foreign key name to constrain
* the target repository instance
*/
constructor(
public getTargetRepository: Getter<TargetRepository>,
public constraint: DataObject<TargetEntity>,
) {}

async create(
targetModelData: DataObject<TargetEntity>,
options?: Options,
): Promise<TargetEntity> {
const targetRepository = await this.getTargetRepository();
// should we have an in memory LUT instead of a db query to increase
// performance here?
const found = await targetRepository.find(
constrainFilter({}, this.constraint),
);
if (found.length > 0) {
throw new Error(
'HasOne relation does not allow creation of more than one target model instance',
);
} else {
return await targetRepository.create(
Copy link
Member

@bajtos bajtos Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is crucial to leverage an atomic implementation of findOrCreate provided by our connectors. The current proposal is prone to race conditions, where to instances of the target "hasOne" model can be created.

Consider the following scenario: the LB4 server is handling two incoming HTTP requests to create a hasOne target and the code is executed in the following way by Node.js runtime:

  1. Request 2 arrives, DefaultHasOneRepository#create is invoked
  2. targetRepository.find is called. It's an async function so other stuff happens while the query is in progress.
  3. Request 1 arrives, DefaultHasOneRepository#create is invoked.
  4. targetRepository.find is called. It's an async function so other stuff happens while the query is in progress.
  5. targetRepository.find for Request 1 returns, no model was found. targetRepository.create in called.
  6. targetRepository.find for Request 2 returns, no model was found. targetRepository.create in called.
  7. Both create requests eventually finish. We have two models that are a target of hasOne relation. 💥 💣 💥

I am proposing to put this pull request on hold and open a new pull request to add findOrCreate method to EntityCrudRepository (and the implementations).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for explaining the need for an atomic implementation with a great example. I agree, and I agree with doing that first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a new issue to keep track of "findOrCreate" story, see #1956

constrainDataObject(targetModelData, this.constraint),
options,
);
}
}

async get(
filter?: Exclude<Filter<TargetEntity>, Where<TargetEntity>>,
options?: Options,
): Promise<TargetEntity | undefined> {
const targetRepository = await this.getTargetRepository();
const found = await targetRepository.find(
Object.assign({limit: 1}, constrainFilter(filter, this.constraint)),
options,
);
// TODO: throw EntityNotFound error when target model instance not found
return found[0];
}
}
2 changes: 2 additions & 0 deletions packages/repository/src/relations/has-one/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@
// License text available at https://opensource.org/licenses/MIT

export * from './has-one.decorator';
export * from './has-one.decorator';
export * from './has-one-repository.factory';
14 changes: 14 additions & 0 deletions packages/repository/src/relations/relation.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,26 @@ export interface BelongsToDefinition extends RelationDefinitionBase {
keyTo?: string;
}

export interface HasOneDefinition extends RelationDefinitionBase {
type: RelationType.hasOne;

/**
* The foreign key used by the target model.
*
* E.g. when a Customer has one Address instance, then keyTo is "customerId".
* Note that "customerId" is the default FK assumed by the framework, users
* can provide a custom FK name by setting "keyTo".
*/
keyTo?: string;
}

/**
* A union type describing all possible Relation metadata objects.
*/
export type RelationMetadata =
| HasManyDefinition
| BelongsToDefinition
| HasOneDefinition
// TODO(bajtos) add other relation types and remove RelationDefinitionBase once
// all relation types are covered.
| RelationDefinitionBase;
Expand Down
18 changes: 18 additions & 0 deletions packages/repository/src/repositories/legacy-juggler-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ import {
createHasManyRepositoryFactory,
BelongsToAccessor,
createBelongsToAccessor,
createHasOneRepositoryFactory,
HasOneDefinition,
HasOneRepositoryFactory,
} from '../relations';
import {resolveType} from '../type-resolver';
import {EntityCrudRepository} from './repository';
Expand Down Expand Up @@ -196,6 +199,21 @@ export class DefaultCrudRepository<T extends Entity, ID>
);
}

protected _createHasOneRepositoryFactoryFor<
Target extends Entity,
TargetID,
ForeignKeyType
>(
relationName: string,
targetRepoGetter: Getter<EntityCrudRepository<Target, TargetID>>,
): HasOneRepositoryFactory<Target, ForeignKeyType> {
const meta = this.entityClass.definition.relations[relationName];
return createHasOneRepositoryFactory<Target, TargetID, ForeignKeyType>(
meta as HasOneDefinition,
targetRepoGetter,
);
}

async create(entity: DataObject<T>, options?: Options): Promise<T> {
const model = await ensurePromise(this.modelClass.create(entity, options));
return this.toEntity(model);
Expand Down
Loading