Preserve custom type of auto-generated id property #4270

frbuceta · 2019-12-06T15:53:16Z

Implements #3602

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 👈

frbuceta · 2019-12-08T18:29:01Z

Can someone help me with the test? It seems not to be happening correctly

derdeka · 2019-12-11T13:41:11Z

Can someone help me with the test? It seems not to be happening correctly

The changes looks reasonable to me and local unit test works successfully.

TASK=test-repository-mongodb and TASK=test-repository-mysql fail in travis ci.
The useDefaultIdType=false seems not to be used by the juggler.
For mysql the following ddl is still created:

CREATE TABLE `User` (
`id` TEXT NOT NULL AUTO_INCREMENT PRIMARY KEY,
`name` VARCHAR(512) NULL,
`roles` TEXT NULL,
`address` TEXT NULL
)

Type TEXT ad AUTO_INCREMENT are not a valid combination.

frbuceta · 2019-12-11T13:45:46Z

Can someone help me with the test? It seems not to be happening correctly

The changes looks reasonable to me and local unit test works successfully.

TASK=test-repository-mongodb and TASK=test-repository-mysql fail in travis ci.
The useDefaultIdType=false seems not to be used by the juggler.
For mysql the following ddl is still created:
CREATE TABLE `User` (
`id` TEXT NOT NULL AUTO_INCREMENT PRIMARY KEY,
`name` VARCHAR(512) NULL,
`roles` TEXT NULL,
`address` TEXT NULL
)
Type TEXT ad AUTO_INCREMENT are not a valid combination.

I certainly did not know that I took code from outside the repository to test, thanks for your help.
As always here I am constantly learning

bajtos · 2019-12-13T15:54:36Z

packages/repository/src/model.ts

@@ -106,6 +106,11 @@ export class ModelDefinition {
    const definition = (definitionOrType as PropertyDefinition).type
      ? (definitionOrType as PropertyDefinition)
      : {type: definitionOrType};
+
+    if (definition.id === true && definition.generated === true) {


To be safe, I am proposing to extend this check to apply useDefaultIdType: false only when an explicit type is provided.

This property should set useDefaultIdType: false:

@property({ type: 'string', id: true, generated: true }) id: string;

This property should NOT set useDefaultIdType: false:

@property({ id: true, generated: true }) id: string | number;

Thoughts?

Yes, it seems right to me. I just implemented it

bajtos · 2019-12-13T15:57:20Z

Thank you @frbuceta for the pull request, this is great!

Type TEXT ad AUTO_INCREMENT are not a valid combination.

Yeah, this is a problem we have recently discovered too, see #2398. I think we need to fix our connectors to correctly support auto-generated string ids (primary keys) before we can move forward with this pull request.

@agnes512 you were looking in to #2398, what do you think?

bajtos · 2019-12-13T16:03:41Z

TASK=test-repository-mongodb and TASK=test-repository-mysql fail in travis ci.
The useDefaultIdType=false seems not to be used by the juggler.

On the second thought, I think this is a bug in the way how our shared test suite is creating models.

The way how this test suite is supposed to work, is that the connector specifies what type is used for primary keys (number vs. string) and the test suite honors that type when defining new models.

I suspect the code building User model is not taking into account database-specific type and always uses string.

AFAICT, User.id property is defined here:

https://github.com/strongloop/loopback-next/blob/4995057ad4c938d6751c329632eb44130ac1567a/packages/repository-tests/src/crud/nested-model-properties.suite.ts#L91-L95

Here is the correct definition of the PK property in a different model:

https://github.com/strongloop/loopback-next/blob/4995057ad4c938d6751c329632eb44130ac1567a/packages/repository-tests/src/crud/create-retrieve.suite.ts#L36-L42

I think we need to add type: features.idType to User.id definition, that should hopefully fix the problem you are observing.

agnes512 · 2019-12-13T16:18:51Z

AUTO_INCREMENT can only apply to type INT. And generated along with useDefaultIdType are not enough for auto-generate string type attribute. We need to rework at connector level to allow so.

I am working on Postgres: allow string type attribute to be auto-generated . That PR is proposing to use uuid_generate_v4() as a default function once the model is set to:

  @property({
    id: true, //optional
    type: 'string'
    generated: true,
    useDefaultIdType: false,
    postgresql: {
      dataType: 'uuid',
    },
  })
  id: String;

I've tested it locally, this solution works in repository-tests with postgresql.

Once it's done, we can rework the MySQL connector with a similar solution. Please share your idea there if you have any 😄

derdeka · 2019-12-13T16:57:01Z

That PR is proposing to use uuid_generate_v4() as a default function

What if someone wants to have a different generator function? (e.g. different UUID version or database extension, different function name)
I think the ddl generation should respect the defaultFn if present.

agnes512 · 2019-12-13T17:47:33Z

What if someone wants to have a different generator function? (e.g. different UUID version or database extension, different function name)

That's doable. It will be user's responsibility to provide valid extension and defaultFn.
And if the extension and defaultFn are not provided at the same time, it'd use uuid_generate_v4(). @derdeka WDYT?

frbuceta · 2019-12-14T16:44:08Z

TASK=test-repository-mongodb and TASK=test-repository-mysql fail in travis ci.
The useDefaultIdType=false seems not to be used by the juggler.

On the second thought, I think this is a bug in the way how our shared test suite is creating models.

The way how this test suite is supposed to work, is that the connector specifies what type is used for primary keys (number vs. string) and the test suite honors that type when defining new models.

I suspect the code building User model is not taking into account database-specific type and always uses string.

AFAICT, User.id property is defined here:

https://github.com/strongloop/loopback-next/blob/4995057ad4c938d6751c329632eb44130ac1567a/packages/repository-tests/src/crud/nested-model-properties.suite.ts#L91-L95

Here is the correct definition of the PK property in a different model:

https://github.com/strongloop/loopback-next/blob/4995057ad4c938d6751c329632eb44130ac1567a/packages/repository-tests/src/crud/create-retrieve.suite.ts#L36-L42

I think we need to add type: features.idType to User.id definition, that should hopefully fix the problem you are observing.

I've tried this but it doesn't work. Right now with the only one I have problems with is mongo.

bajtos · 2020-01-07T13:46:29Z

packages/repository/src/model.ts

+    if (
+      definition.id === true &&
+      definition.generated === true &&
+      definition.type === 'string'


I am afraid we misunderstood each other. Here is what I meant:

Suggested change

definition.type === 'string'

definition.type !== undefined

Either way, please add a new test to verify that useDefaultIdType is not set when definition.type was not provided (and was not inferred from TypeScript metadata either).

There is no model in the tests of the type as undefined, how should I generate a model with these features?

packages/repository/src/model.ts

bajtos · 2020-01-07T13:50:12Z

TASK=test-repository-mongodb and TASK=test-repository-mysql fail in travis ci.

On the second thought, I think this is a bug in the way how our shared test suite is creating models.
I think we need to add type: features.idType to User.id definition, that should hopefully fix the problem you are observing.

I've tried this but it doesn't work. Right now with the only one I have problems with is mongo.

Hmm...

@agnes512 IIRC, you were dealing with similar issues recently, while working on inclusion resolvers. Can you please help @frbuceta to find a way how to make repository tests pass against MongoDB?

frbuceta · 2020-01-07T16:34:45Z

Thanks for your help @agnes512

packages/repository/src/model.ts

agnes512 · 2020-01-07T20:49:13Z

...ests/src/crud/relations/acceptance/multi-relations-inclusion-resolver.relation.acceptance.ts

      });
      const order = await orderRepo.create({
        description: 'an order',
        customerId: parent.id,
      });

      const result = await customerRepo.find({
-        include: [{relation: 'orders'}, {relation: 'customers'}],


While I was fixing the CI failure, I think the flag might somehow break how we deal with ObjectId in MongoDB. Before we were able to traverse related instances with ObjectId. Not sure it has something to do with the type.

bajtos · 2020-02-17T13:50:59Z

@agnes512 @frbuceta what's the status of this pull request? I see that the CI is green and the code looks reasonable, even if not all edge cases may be covered by tests. Should we perhaps proceed to land the changes as they are now?

frbuceta requested review from bajtos and raymondfeng as code owners December 6, 2019 15:53

bajtos added the community-contribution label Dec 9, 2019

bajtos reviewed Dec 13, 2019

View reviewed changes

bajtos mentioned this pull request Dec 13, 2019

Column is always integer when running npm run migrate #2398

Closed

bajtos reviewed Jan 7, 2020

View reviewed changes

bajtos assigned bajtos and agnes512 Jan 7, 2020

agnes512 reviewed Jan 7, 2020

View reviewed changes

packages/repository/src/model.ts Show resolved Hide resolved

agnes512 reviewed Jan 7, 2020

View reviewed changes

achrinza added the Repository Issues related to @loopback/repository package label Jan 19, 2020

dougal83 mentioned this pull request Jan 27, 2020

Mysql string id is null #4512

Closed

dhmlau mentioned this pull request Jan 30, 2020

Monthly Milestone - February 2020 💝 🔆 ☕️ #4543

Closed

33 tasks

bajtos requested a review from hacksparrow February 17, 2020 13:48

bajtos removed their assignment Feb 17, 2020

feat: preserve custom type of auto-generated id property

d895a9e

agnes512 approved these changes Feb 18, 2020

View reviewed changes

agnes512 merged commit dc7ff7f into loopbackio:master Feb 18, 2020

agnes512 mentioned this pull request Feb 18, 2020

Preserve custom type of auto-generated id property #3602

Closed

frbuceta deleted the feat/preserve-custom-id-type branch February 27, 2020 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve custom type of auto-generated id property #4270

Preserve custom type of auto-generated id property #4270

frbuceta commented Dec 6, 2019

frbuceta commented Dec 8, 2019

derdeka commented Dec 11, 2019

frbuceta commented Dec 11, 2019

bajtos Dec 13, 2019

frbuceta Dec 14, 2019

bajtos commented Dec 13, 2019

bajtos commented Dec 13, 2019

agnes512 commented Dec 13, 2019

derdeka commented Dec 13, 2019

agnes512 commented Dec 13, 2019 •

edited

Loading

frbuceta commented Dec 14, 2019

bajtos Jan 7, 2020

frbuceta Jan 7, 2020

bajtos commented Jan 7, 2020

frbuceta commented Jan 7, 2020

agnes512 Jan 7, 2020

bajtos commented Feb 17, 2020

Preserve custom type of auto-generated id property #4270

Preserve custom type of auto-generated id property #4270

Conversation

frbuceta commented Dec 6, 2019

Checklist

frbuceta commented Dec 8, 2019

derdeka commented Dec 11, 2019

frbuceta commented Dec 11, 2019

bajtos Dec 13, 2019

Choose a reason for hiding this comment

frbuceta Dec 14, 2019

Choose a reason for hiding this comment

bajtos commented Dec 13, 2019

bajtos commented Dec 13, 2019

agnes512 commented Dec 13, 2019

derdeka commented Dec 13, 2019

agnes512 commented Dec 13, 2019 • edited Loading

frbuceta commented Dec 14, 2019

bajtos Jan 7, 2020

Choose a reason for hiding this comment

frbuceta Jan 7, 2020

Choose a reason for hiding this comment

bajtos commented Jan 7, 2020

frbuceta commented Jan 7, 2020

agnes512 Jan 7, 2020

Choose a reason for hiding this comment

bajtos commented Feb 17, 2020

agnes512 commented Dec 13, 2019 •

edited

Loading