Strict typing on create() doesn't account for generated fields

[rjmackay]

Describe the bug

This change: https://github.com/sensedeep/dynamodb-onetable/pull/339 changed the typedef for Model.create() to enforce required model properties during create. This makes sense, except if you have a required property that's also generated.

For example, we have a property like this

      consumerId: { type: String, generate: true, required: true },

The new type schema requires I manually input consumerId and gives an error Property 'consumerId' is optional in type 'Partial<Consumer>' but required in type '{ consumerId: ConsumerId; ...

To Reproduce

I'm not entirely sure how to give you a repro for a type issue. I can try if this isn't clear though.

Expected behavior Generated fields should be marked required in the Model type, but not in the input to Model.create

Screenshots If applicable, add screenshots to help explain your problem.

Environment (please complete the following information):

• OS: Mac OS
• Node Version: 14.18
• OneTable Version: 2.3.8
• TypeScript Version: 4.6.3

[mobsense]

A minimal debug.ts would save us the time of creating one.

[mobsense]

I'm looking into this.

An earlier PR got reverted that typed the create properties as: CreateProperties<T>

This was not working where it would not flag any missing required field.

But the alternative is flagging missing generated properties.

What we need is a fixed CreateProperties<T> definition.

I'm digging into the TS for this, but this has some unholy magic involved. Any help would be appreciated.

[mobsense]

The fix may be changing in Model.d.ts:

type CreateProperties<T> = Omit<T, keyof Generated<ExtractModel<T>>> | Generated<ExtractModel<T>>

to

type CreateProperties<T> = Omit<T, Generated<ExtractModel<T>>>

And create() to:

create(properties: CreateProperties<T>, params?: OneParams): Promise<T>;

[mobsense]

Any feedback on the above suggestion, greatly appreciated.

[mobsense]

I'm digging more into this and I think there is some confusion regarding what does "required" mean.

Required means that you must provide the property when creating.

So you should not use required on a 'generate' property as OneTable is providing the value and you do NOT have to provide it to create.

Some general rules:

• Only flag properties as required that must be provided by the caller of create
• Don't set required when using 'default'. Either set required and have the caller provide the data or use default if it is optional.

With these assumptions, CreatedProperties<T> can go away and just be replaced with T.

What am I missing?

[shishkin]

How would you then represent nullability of a field in the schema? I'm using TypeScript and would like to be able to use onetable's Entity<> types as my domain entities and not to having to check attributes for null and undefined every time I get an entity from a DB.

[mobsense]

Back to the original issue.

Yes, there is a bug in handling create properties that are required and generated. The TS handling requires that you provide the generated property to satisfy the type signature. This is not ideal. We're working on more comprehensive fix for this. See discussion #

---

Regarding nullability in general.

The Table params "nulls" controls how nulls are handled. Generally this is set to false, meaning, never write nulls to the database. With nulls == false, when a property is null and Model.update() is called, OneTable will remove the property from the item. It would be nice if our typings for Update() allows nulls and other APIs did not.

With TS, nullability gets somewhat overloaded. What does null really mean in your app? It varies from app to app. Does it mean the property is not present in the database or that it is present with a null value. i.e. Do you want to store null in the database?

So we should separate the concerns of what gets stored in the Database, vs Entity types that your app uses.

We generally run with Table.parms.nulls == false and never store nulls in the database. This greatly simplifies things.

Now, we need OneTable TS support to not allow nulls in Entity types (but perhaps only if params.nulls == false).

[shishkin]

What does null really mean in your app?

I also don't use null in my app at all. I always use strictNullChecks. If I need to express absence of value I use undefined and add it to type signature accordingly: name: string | undefined or name?: string.

Do you want to store null in the database?

No. And I'm happy with the default behavior of params.nulls: false.

I would also prefer if onetable didn't return null values for fields absent in the DB including nested Object fields.

And I would rather prefer update() rely on params.remove: [name] explicit syntax to remove fields instead of fiddling with nulls.

[mobsense]

Great, thanks.

We're toiling at the moment trying to get TS magic to work on create().

[mobsense]

Okay, we have it working sweetly. This is some serious TS magic. Ready for test.

The new magic does:

• Fixed create(), find() and update() to correctly flag incorrect properties.
• Fixed lax use of nulls. Now only permitted in update() to delete properties.
• Correctly visually highlights in VS code when you have incorrect properties or types in data items.
• Correctly highlights methods arguments for create(), find(), update etc. Will warn when required parameters are missing and when data types are incorrect.
• Correctly autosuggests data field property names as you code.

The Schema Rules

• Required in a schema model field means the field is required in data items. A model type will have these as required properties. Required means an item object MUST have the property present. Don't confuse this with the properties you give to create/update/get/find etc. Those are NOT model data types. They have special rules.
• Without required, the property will be optional. Default, generated, value-templates can be marked as required.
• In create() you do not have to provide generated fields, fields with default values or fields with a value template ... Even if they are flagged as required.
• In find() you can provide any subset of the properties (provide it is valid for DynamoDB of course).
• In update() you can provide any subset of the properties AND you can set a properties value to null to remove it.
• In update() you can supply sparse nested objects if you have set partial: true to the table constructor params. Update will update only the properties you have provided, regardless of how deep.

Notes

• You can now use Table.getModel in Typescript without <>. i.e.

Use

const User = table.getModel(‘User’)

rather than

const User = table.getModel<UserType>('User')

The Bad

The intellisense (IS) on methods is pretty complex. However the IS will correctly prompt when typing for data fields and the IS on data types is great.

Questions?

[vhuty]

@mobsense seems like the updated and created timestamp fields are still required when calling Model.create(...). Take a look at this schema:

{
  format: "onetable:1.1.0",
  version: "0.0.1",
  indexes: {
    primary: { hash: "PK", sort: "SK" },
  },
  params: {
    typeField: "_type",
    timestamps: true,
    isoDates: true,
    createdField: "createdAt",
    updatedField: "updatedAt",
  },
  models: {
    Order: {
      PK: { type: String, value: "id#${id}", hidden: true },
      SK: { type: String, value: "id#${id}", hidden: true },
      id: { type: String, required: true },
      createdAt: { type: Date, required: true },
      updatedAt: { type: Date, required: true },
    },
  },
}

Didn't find it in the documentation: if I specify params.createdField: createdAt, should I remove this field from the models definition?

[shishkin]

Didn't find it in the documentation: if I specify params.createdField: createdAt, should I remove this field from the models definition?

Yes, you don't need to specify timestamp fields in the model.

[dev-embedthis]

Your choice. You don't have to specify them in the model, but if you are using TS and want intellisense, then define them in your model.

[vhuty]

@dev-embedthis if I remove these fields from the model, will they be considered as "metadata" and won't they be returned from methods such as Model.get?

[dev-embedthis]

The timestamp fields are always returned. Probably should respond to the "hidden" param and not be returned by default.

But currently, they are returned.

Changing would probably break some existing code.

[vhuty]

Since timestamp fields are always returned, shouldn't they be in the current type definitions if they're not specified in the model (just in createdField/updatedField)?

[dev-embedthis]

To do that, just define them in your model. TS definitions are auto-generated from your schema, so if you want them defined, you need to include them in your model.