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

Issue with .addToCollection() with string IDs #4591

Closed
shobhitsinghal624 opened this issue Feb 18, 2019 · 7 comments
Closed

Issue with .addToCollection() with string IDs #4591

shobhitsinghal624 opened this issue Feb 18, 2019 · 7 comments
Labels
inconsistency orm Related to models, datastores, orm config, Waterline, sails-hook-orm, etc.

Comments

@shobhitsinghal624
Copy link

shobhitsinghal624 commented Feb 18, 2019

Sails version: 1.1.0
Node version: v10.12.0
NPM version: 6.8.0
DB adapter name: N/A
DB adapter version: N/A
Operating system: macOS


Calling .addToCollection() where id is defined as a string (specifically a UUID) throws error. It works perfectly when id is a number.

Steps to reproduce the issue provided below.

Create models with many-to-many association:

// api/models/User.js
module.exports = {
  attributes: {
    name: {
      type: 'string',
      required: true,
    },
    teams: {
      collection: 'Team',
      via: 'members',
    },
  },
};
// api/models/Team.js
module.exports = {
  attributes: {
    name: {
      type: 'string',
      required: true,
    },
    members: {
      collection: 'User',
      via: 'teams',
    },
  },
};

Set id to be a UUID in config/models.js

// config/models.js
module.exports.models = {
  schema: true,
  migrate: 'safe',
  attributes: {
    createdAt: { type: 'number', autoCreatedAt: true, },
    updatedAt: { type: 'number', autoUpdatedAt: true, },
    id: { type: 'string', required: true, unique: true, isUUID: true, },
  },
  dataEncryptionKeys: {
    default: 'PQkpGh5U7G33kR/6wuG/ns81ad2ls24Bb6sohYkNr4U='
  },
  cascadeOnDestroy: true
};

Finally, make these calls in sails console to reproduce the issue. The .addToCollection() call (last one) throws an error.

$ sails console

 info: Starting app in interactive mode...

 info: Welcome to the Sails console.
 info: ( to exit, type <CTRL>+<C> )

sails> User.create({ id: '368d42a8-86b5-4edc-9049-fd87895ba0cb', name: 'Bruce Wayne' }).fetch().exec(console.log)
undefined
sails> undefined { createdAt: 1550467916777,
  updatedAt: 1550467916777,
  id: '368d42a8-86b5-4edc-9049-fd87895ba0cb',
  name: 'Bruce Wayne' }

sails> User.create({ id: '66d92317-cd0a-42af-99cf-665aeb85f512', name: 'Diana Prince' }).fetch().exec(console.log)
undefined
sails> undefined { createdAt: 1550467921900,
  updatedAt: 1550467921900,
  id: '66d92317-cd0a-42af-99cf-665aeb85f512',
  name: 'Diana Prince' }

sails> Team.create({ id: '23ff4ff2-f6d1-49f4-a12a-eb67c275a0e8', name: 'Justice League' }).fetch().exec(console.log)
undefined
sails> undefined { createdAt: 1550467927067,
  updatedAt: 1550467927067,
  id: '23ff4ff2-f6d1-49f4-a12a-eb67c275a0e8',
  name: 'Justice League' }

sails> Team.addToCollection('23ff4ff2-f6d1-49f4-a12a-eb67c275a0e8', 'members').members(['368d42a8-86b5-4edc-9049-fd87895ba0cb', '66d92317-cd0a-42af-99cf-665aeb85f512']).exec(console.log)
{ UsageError: Invalid initial data for new records.
Details:
  Could not use one of the provided new records: Missing value for required attribute `id`.  Expected a string, but instead, got: undefined
 [?] See https://sailsjs.com/support for help.
    at /Users/shobhit.singhal/workspace/sails-bug-report-app/node_modules/waterline/lib/waterline/methods/add-to-collection.js:398:19
    at Deferred._.extend._WLModel [as _handleExec] (/Users/shobhit.singhal/workspace/sails-bug-report-app/node_modules/waterline/lib/waterline/methods/add-to-collection.js:438:9)
    at Deferred.exec (/Users/shobhit.singhal/workspace/sails-bug-report-app/node_modules/parley/lib/private/Deferred.js:286:10)
    at repl:1:163
    at Script.runInThisContext (vm.js:96:20)
    at REPLServer.defaultEval (repl.js:329:29)
    at bound (domain.js:396:14)
    at REPLServer.runBound [as eval] (domain.js:409:12)
    at REPLServer.onLine (repl.js:642:10)
    at REPLServer.emit (events.js:187:15)
    at REPLServer.EventEmitter.emit (domain.js:442:20)
    at REPLServer.Interface._onLine (readline.js:290:10)
    at REPLServer.Interface._line (readline.js:638:8)
    at REPLServer.Interface._ttyWrite (readline.js:919:14)
    at REPLServer.self._ttyWrite (repl.js:715:7)
    at ReadStream.onkeypress (readline.js:168:10)
    at ReadStream.emit (events.js:182:13)
    at ReadStream.EventEmitter.emit (domain.js:442:20)
    at emitKeys (internal/readline.js:424:14)
    at emitKeys.next (<anonymous>)
    at ReadStream.onData (readline.js:1022:36)
    at ReadStream.emit (events.js:182:13)
  name: 'UsageError',
  code: 'E_INVALID_NEW_RECORDS',
  details:
   'Could not use one of the provided new records: Missing value for required attribute `id`.  Expected a string, but instead, got: undefined' }
undefined
sails>
@sailsbot
Copy link

@shobhitsinghal624 Thanks for posting, we'll take a look as soon as possible.


For help with questions about Sails, click here. If you’re interested in hiring @sailsbot and her minions in Austin, click here.

@johnabrams7 johnabrams7 added the orm Related to models, datastores, orm config, Waterline, sails-hook-orm, etc. label Feb 18, 2019
@raqem
Copy link
Contributor

raqem commented Feb 18, 2019

HI @shobhitsinghal624,
I was able to reproduce your issue. I am looking at it with @mikermcneil
We'll let you know when we find something!

@raqem raqem added the bug label Feb 18, 2019
@mikermcneil
Copy link
Member

@shobhitsinghal624 @raqem Ok, so here's the problem:

The default attributes configured in config/models.js are applied to all models-- even the hidden junction models that power many-to-many associations:

These attributes will be available in all models, unless they are overridden or disabled.

image

In other words, Waterline doesn't know what UUID to use when creating junction records.

I think there are two solutions:

  1. Recommended: Use an auto-incrementing number for your ids by default. But then, on models where you'd like to use a required, manually-specified UUID, define an id attribute the way you did above. This way, Waterline can automatically create join records for you, and you still get to use UUIDs for the records that actually matter to your business logic.
  2. You could make your junction model explicit (e.g. create a TeamMembership model), and replace your many-to-many association between users and teams with a one-to-many association between users and memberships, and a one-to-many association between teams and memberships. (In other words, a team can have many memberships, and so can a user.)

I'll have a think about other ways we could improve this in core so that it just works™, but in the mean time I think that's your best bet.

Thanks for bringing this up!

@raqem raqem added inconsistency and removed bug labels Feb 18, 2019
mikermcneil added a commit to balderdashy/waterline that referenced this issue Feb 18, 2019
@raqem
Copy link
Contributor

raqem commented Feb 18, 2019

Hey @shobhitsinghal624,
I just wanted to follow up and let you know that I tried out @mikermcneil first suggestion above and was able to add members to my team.
image

Here is my code:

config/models.js

attributes: {
    createdAt: { type: 'number', autoCreatedAt: true, },
    updatedAt: { type: 'number', autoUpdatedAt: true, },
    id: { type: 'number', autoIncrement: true,},

And in both User.js and Team.js I added an attribute for id

id: {
      type: 'string',
      required: true,
      unique: true,
      isUUID: true,
    },

Hope this helps!

@shobhitsinghal624
Copy link
Author

Thanks @raqem, @mikermcneil !
If I understand the issue correctly, the junction record isn't getting created because

  1. it inherits from default attributes configured in config/models.js which makes the id of the junction record a UUID, but
  2. there is no way for waterline to know what UUID value to assign when creating the junction record.

In my head (1) is a correct implementation, and we should solve (2). Maybe overload defaultsTo functionality to accept a generic id generator?
That said, I'll go ahead and use your recommended solution for now.

Closing the issue now. Thanks again!

@bbmattieu9
Copy link

Hello I do not know if my problem is the same, i have tags coming my frontEnd formData as an array of IDs. In sails i attached the array to Articles collection using the addToCollection(). The response from sails is that UsageError: The associated ids (i.e. using members(), or the third argument) passed to .addToCollection() should be the ID (or IDs) of associated records to add.`

@bbmattieu9
Copy link

Details:
  The provided value is not valid primary key value.  Instead of a number, the provided value (`'[27,26,25]'`) is a string, and it cannot be coerced into a valid primary key value automatically (contains characters other than numerals 0-9).  To resolve this error, pass in a valid base-10, non-zero, positive integer instead.  (Or if you must use strings, then change the relevant model's pk attribute from `type: 'number'` to `type: 'string'`.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inconsistency orm Related to models, datastores, orm config, Waterline, sails-hook-orm, etc.
Development

No branches or pull requests

6 participants