Throw exception if accessing primary key on unflushed entity

[parisholley]

Is your feature request related to a problem? Please describe.
If a developer is unaware that an entity has been passed to them without an id, they may end up attempting to take action on it (eg: serialize to JSON) and never catch it, especially since there is no compile-time type checking to protect against this.

Describe the solution you'd like
When an entity is created through manager.create(), intercept calls to the primary key prior to flush and throw an exception, telling the user they need to flush first.

Describe alternatives you've considered
Could potentially have a base entity class with get id() and set id() which includes some check, though it isn't clear what the right approach is for determining "flushed", isInitialized?

This is what I am using now as a workaround:

export default abstract class BaseEntity {
  @Field(() => ID, { name: 'id' })
  private _id: string;

  get id(): string {
    if (!this._id) {
      const error = new Error(`Entity has not been given a id yet, please flush.`);

      const stack = error.stack.split('\n');

      if (stack[2].includes('@mikro-orm')) {
        // hack to allow mikro to process inside unit of work
        return undefined;
      }

      throw error;
    }

    return this._id;
  }

  @PrimaryKey({ type: BigIntType, fieldName: 'id', getter: true })
  private set id(value: string) {
    this._id = value;
  }
}

[B4nan]

Not sure if I like this enough to make it default, will need to think about it a bit more (but it's definitely an interesting idea). I'd call this a breaking change for sure, and it should definitely be implemented in a way so a constrcutor calls will trigger that too, em.create() is not the only way to create entities (for me it's not even the suggested way, I do suggest using constructors directly every time I can). Using base entities is not required as well, so only way to implement this would be altering the entity prototype dynamically - which already happens anyway for relations to support propagation.

We could make this configurable, as I can imagine codebases where people check for the PK value explicitly - such code would now throw and require refactoring

though it isn't clear what the right approach is for determining "flushed", isInitialized?

Not really, initialized means that you have the data and not just PK, e.g. a naked reference is not initialized but it has a PK. You could check the PK value - but that can be also set without entity being flushed (e.g. UUID). To get around that, you'd have to also check if the identity map has such entity. The question is whether it makes sense to be so strict, e.g. you want to read the PK, the value is there, but we are not flushed.

[parisholley]

for me it's not even the suggested way, I do suggest using constructors directly every time I can

i started using em.create/em.assign mostly because it was more ergonomic to define the relations without having to introduce additional api calls/lines (especially for ingesting user data, eg: graphql) for getting references and rather not have two conventions in the app. one potential solution to get away from em.create, would be if we could use new Reference() (like we do new Collection()) in our entities, that way we could just do something like entity.reference.set(1), but that would replace em.assign for input scenarios?

altering the entity prototype dynamically

good point, though i'd imagine that only works if you send the instance to EM ( in which case, a base entity is helpful)? Or are you saying at bootstrap, you update the prototype for the entity class?

people check for the PK value explicitly

example use case?

The question is whether it makes sense to be so strict, e.g. you want to read the PK, the value is there, but we are not flushed.

yeah, that would be too strict, assuming the meta/entity is configured to persist that PK vs replace it with an auto-generated one, it would be safe to use the id downstream. the concern is just making sure we don't pass undefined downstream by accident.

Not really, initialized means that you have the data and not just PK

right, and a new unflushed entity is technically initialized, so there would need to be some way to check if the primary key is to be generated and has not been provided

[B4nan]

i started using em.create/em.assign mostly because it was more ergonomic to define the relations without having to introduce additional api calls/lines (especially for ingesting user data, eg: graphql) for getting references and rather not have two conventions in the app.

It's fine to use them, it definitely makes sense with extensive usage of wrapped references as it can save a lot of code. What I meant is that I don't want to have a solution just for em.create(), it's definitely a valid approach.

one potential solution to get away from em.create, would be if we could use new Reference() (like we do new Collection()) in our entities, that way we could just do something like entity.reference.set(1), but that would replace em.assign for input scenarios?

I am fine with that as long as it would be optional. Not sure I understood the note about em.assign() - if you wanted to say what would instead of that would, then my note before was just about em.create() :]

you update the prototype for the entity class?

Yes, during entity discovery the prototypes are patched. That is also the reason why entity discovery must always happen even in unit tests (while you can disable connection to database if you don't plan to use it).

so there would need to be some way to check if the primary key is to be generated and has not been provided

Actually there is much easier way than using the identity map check I suggested before. You can simply check if wrap(entity, true).__originalEntityData is empty or not. Only managed entities have that and it is updated after flush.

example use case?

Well, exactly yours, isn't it? You want to know whether the PK is there. You are suggesting to throw when it's not there, people might be checking in if statement currently, if they somewhere know it can be unflushed entity.

[parisholley]

Well, exactly yours, isn't it?

i wouldn't say so, i don't think the following code should ever exist in most projects unless you are generating ids your self and want to check for the existence of it (which could be argued is the purpose of onCreate or some other hook).

if(!entity.id){
 // do x y z
}

i can't think of an instance where a method would take an entity argument under the assumption that id would be undefined and alter execution behavior. add to the fact that i think most people are going to model their types like id: string or id!:string and never id?: string, you wouldn't instinctively be checking for id existence anyway.

[B4nan]

I am quite sure I saw such snippets either here in issues or on slack, and even if I haven't, I still consider this a breaking change - previously such code could exist and now the behaviour would change (of course only in case we enable this by default). But either way, I am not planning any more feature releases to v4, only bug fixes.

[parisholley]

another thought on this, what if we introduce a new type similar to IdentifierReference, like FuturePrimaryKey<string>/PersistedPrimaryKey<string>? this way, you can opt-in to the behavior and if you call .get(), it would error, otherwise, you can await id.generate() which will cause a flush. this would also simplify some of the GetReference API and you can just pass the primary key references around

[B4nan]

You don't have the right EM instance from inside new entities. I know you are using em.create() but that is optional, with constructors we don't have a way to get the right EM. And to trigger a flush, you need to know it. As opposed to references and collections, where you can load them only on managed entities, here you'd like to trigger the flush on a new one.

[parisholley]

ah yes... only other thing that comes to mind is some type of flush callback system. eg:

em.onFlush(entity, (entity) => {
  em.create(other, {
   metadata: {entity.id}
  });
});

[B4nan]

Having the patched prototype seems like the best way to me.

[parisholley]

@B4nan trying to figure out whether to build a solution or close and live with monkey patch. is it feasible to introduce a "wrapped" identifier option in the code base (excluding any need for flushing/having a reference to the EM) that would look like this:

export class Entity {
  @PrimaryKey({ type: BigIntType, wrapped: true })
  private id = new GeneratedPrimaryKey<string>();
}

[B4nan]

I was recently fixing one issue with the promise.all and flushing resulting in flaky tests, and introduced async local storage context to the flush call to deal with it. We could use the same context, it literally tells you if you are inside the flush:

https://github.com/mikro-orm/mikro-orm/blob/master/packages/core/src/unit-of-work/UnitOfWork.ts#L330

Haven't read the whole thread again, but I guess this should do it? We don't need to know the right EM, we just need to know if the current async context is inside flush handler, right?

[parisholley]

I think i'm ok with not worrying about flush yet, having to make downstream consumers async just to ensure an id exists seems iffy. and just throwing is at least a simpler v1 approach