Non-integer primary keys are forced to the model's id during make/build

[mdentremont]

Given:

• A model, having a string type as the primary key

When generating the model with factory guy, the primary key attribute which should be a string is replaced by the integer id.

Code:

// app/models/some-model.js
export default DS.Model.extend({
  value: DS.attr('string'),
  label: DS.attr('string')
});

// app/serializers/some-model.js
import DRFSerializer from './drf'; // Issue should be present on all serializers

export default DRFSerializer.extend({
  primaryKey: 'value'
});

// tests/factories/some-model.js
FactoryGuy.define('some-model', {
  default: {
    value: faker.lorem.word(),
    label: faker.lorem.word()
  }
});

I believe there to be two issues here:

1. a string is not used for the primary key attribute if if the model uses DS.attr('string')
2. the factory definition for the primary key field is thrown away

Some investigation shows that this is likely due to addPrimaryKey being called as the last step for make/build. This function does not check for the primary key's type, and also does not check to see if it is already set:

   addPrimaryKey(modelName, data, fixture) {
     let primaryKey = this.store.serializerFor(modelName).get('primaryKey');
     if (fixture.id) {       // might need to check the id field type when this was originally set
       data.id = fixture.id;
       if (primaryKey !== 'id') {      // maybe we should instead be setting the value of fixture.id to data[primaryKey] if it is set?
         data[primaryKey] = fixture.id;
       }
     }
   }

[danielspaniel]

hmm .. this is a puzzler
I see what you mean.
Wondering why I am adding primary key here instead of in the modelDefinition class

you want factoryGuy to build a fixture,
set the value ( you using faker )
don't bother to set an id ( since it's not necessary, as your using value as the id )

you and whoever does this are responsible that id ( value in your case ) is created and is unique ..

is that right?

[mdentremont]

Agreed (I think): If a fixture has a definition for the ID field, I think it would be the responsibility of the fixture owner to make it unique the value be unique

[danielspaniel]

Hmm .. this brings up the issue where you probably don't need the id for an id .. but is nice to have for ability to make fixture data like

// tests/factories/some-model.js
FactoryGuy.define('some-model', {
  default: {
    value: (f) => `custom_string_id${f.id}`,
    label: faker.lorem.word()
  }
});

but really you are using it as an index .. or a sequence number so you don't have to generate a sequence object.

let me ponder this to see if I can call it num so you can use it like f.num or maybe f.idx and it is clear it is just a free index number and NOT the id

thoughts?

[mdentremont]

@danielspaniel Yeah that sounds good --- also don't mind the usage of faker above, I'm not deadset on it, just using it as sample code.

[danielspaniel]

Sure thing @mdentremont ... I was showing how the id is so useful when making fixtures and was tying to show how even without an id field it is still useful for making fake data.
I don't use faker but I don't mind it really.
I always like to see what the field is like "Name 1" or "Value 1" and whatnot.

[danielspaniel]

@mdentremont , v2.7.1 has this new and improved edge case covered