Support nested objects and arrays in the model definition

[gidesan]

Adds support to nested objects and arrays in model definitions, e.g.

  const db = factory({
    user: {
      id: primaryKey(datatype.uuid),
      name: String,
      info: {
        firstName: String,
        lastName: String,
        address: {
          street: () => 'Yellow Brick Road',
          number: () => 1,
        },
        tags: Array,
      },
    },
  })

This PR is expected to solve this issue #97 as well.

[kettanaito]

Hey, @gidesan. Thank you for working on the nested objects support in the models!

I've done the code review and have a few questions and clarifications that I'd appreciate you answering. Overall, I think the changes are good, I just need to get my ahead around flattenning the nested object keys and handling them like pointers. I wonder if there's a more efficient way to this, so we don't have to flatten potentially deep data structures.

[kettanaito]

If you update against the latest main branch, you'll get isObject assertion in the isInternalEntity and can remove the custom logic added here. Thanks!

[gidesan]

If you update against the latest main branch, you'll get isObject assertion in the isInternalEntity and can remove the custom logic added here. Thanks!

Did this as well. Hope I didn't mess up the git history

[kettanaito]

Hey, @gidesan. Thank you once more for covering my suggestions!

I do have some concerns about some API changes in the parsing function, but I don't wish to block you while I think of a way to solve them. There are a few comments regarding moving the tests and reusing existing utilities that you may address if you have time.

[kettanaito]

If you're comfortable with Git, we should clean up the commit history for this change. Each commit should represent a finite meaningful change. For example:

• chore: migrate to "lodash" package
• feat: support nested objects and arrays in the model definition
• ...

You can leave this formatting to me if you feel unsure. We can rebase the commits once the change is finished.

[gidesan]

If you're comfortable with Git, we should clean up the commit history for this change. Each commit should represent a finite meaningful change. For example:

• chore: migrate to "lodash" package
• feat: support nested objects and arrays in the model definition
• ...

You can leave this formatting to me if you feel unsure. We can rebase the commits once the change is finished.

Tbh I used not-so-meaningful commit messages thinking that we would have squashed them in the end. Even my email should be changed in the commits.
I could try to end the work, then generate a patch, then create a new branch and split the patch in meaningful commits with proper comments. A bit boring, but I can do that.

[gidesan]

Hey, @gidesan. Thank you once more for covering my suggestions!

I do have some concerns about some API changes in the parsing function, but I don't wish to block you while I think of a way to solve them. There are a few comments regarding moving the tests and reusing existing utilities that you may address if you have time.

I should have addressed your new suggestions, thanks a lot :)

[jogelin]

Really impatient for this PR to be merged :)

We would like to use this library to generate all factories for all of our mocks in cypress/jest testing and for localhost development too. However not having primitive arrays is blocking us for choosing this library :s

[kettanaito]

I've squashed the commits and rebased the branch so it's up-to-date with the main one. I'll give it a final round of review and then it should be ready to go. Thank you for being patient.

[kettanaito]

What was your intention behind adding this explicit isFunction check?

Type-wise, at this point of processing propertyDefinition is a base type getter (() => BaseType), in other words: always a function. Perhaps you wanted to guard this reduce against malformed user input?

[gidesan]

After 2 months I don't exactly remember tbh. Probably what you guessed, otherwise something related to the very first implementation of the feature. It's probably redundant at this point.

Btw was just looking at line 66, where I wonder if logging "has a plain initial value" is still true for arrays (which now are an option)

[kettanaito]

No worries, I'm remembering everything around this myself.

looking at line 66, where I wonder if logging "has a plain initial value" is still true for arrays

Is it the wording that confuses you? I'd say it matters little as it's internal logging.

[kettanaito]

There's an issue with properly typing nested objects in the ModelDefinition type. We need this to provide TypeScript support for the users that will start declaring nested objects in their models after this feature.

Ideally, we should recursively reference that type, so to support infinitely nested objects:

export type ModelDefinitionValue =
  | PrimaryKeyDeclaration
  | OneOf<any>
  | ManyOf<any>
  | (() => PrimitiveValueType)

export type ModelDefinition = {
  [propertyName: string]: ModelDefinitionValue | ModelDefinition
}

Once we do so, however, there's no way to reliably differentiate between internal types like OneOf or PrimaryKeyDeclaration and an arbitrary object provided by the user. Here's what I mean:

factory({
  user: {
    address: {
      isPrimaryKey: true,
      getValue: () => 'abc-123'
    }
  }
})

Such a user model will treat user.address as its primary key, bypassing the primaryKey utility that's recommended to mark model properties as primary keys. In general, any internal complex value, like relation or a primary key, is represented via an object to contain additional information about that value (the value of a primary key, or the type/uniqueness of a relation):

data/src/glossary.ts

Lines 19 to 24 in 3016d6d

	export interface PrimaryKeyDeclaration<
	ValueType extends PrimaryKeyType = string
	> {
	isPrimaryKey: boolean
	getValue(): ValueType
	}

data/src/glossary.ts

Lines 44 to 49 in 3016d6d

	export interface Relation {
	kind: RelationKind
	modelName: string
	unique: boolean
	primaryKey: PrimaryKeyType
	}

Once we introduce arbitrary objects as values, it'll be impossible to differentiate between them and internal values. That's already evident if we implement the ModelDefinition type change as suggested above—you'll see how parseModelDefinition will struggle when assigning relations, as the value becomes a union of ModelDefinition | OneOf<any>, both being objects.

Even if we introduce distinct properties in internal values to clearly detect them (as we have with isPrimaryKey), it will solve one issue and result in another: now arbitrary objects may be treated as internal values given some of those distinct keys match (unintentionally).

I think there's a couple of options we have here.

Option 1: Use distinct properties on internal values

We may use unlikely property names to partially account for this, like __kind in a relational node value, but it won't ever be bulletproof, users may still run into issues if their keys collide with these distinct keys.

Option 2: Treat internal values as class instances

Internal values (relations/primary keys) may be represented as class instances. That way we can leverage class belonging to eliminate unintentional property collisions with internal values' properties.

class PrimaryKey { ... }

function primaryKey(...args) {
  return new PrimaryKey(...args)
}

// parseModelDefinition.ts
if (value instance of PrimaryKey) {
  // treat the value as a primary key
}

if (value instanceof Relation) {
  // treat the value as a relation
}

This would add a little clutter internally, but should allow us to reliably differentiate between internal and arbitrary values in the context of nested objects support.

@gidesan, I'd love to hear your feedback on this. Please do not proceed with implementation until we discuss this in sufficient detail.

[kettanaito]

Meanwhile, I've pushed some minor adjustments to the code, mainly around the recursive parseModelDefinition. It should be much more concise now, leveraging a single result pointer shared between main and recursive branches of the function (no need to manually merge relations/properties for nested objects anymore). All tests are passing.

[kettanaito]

I'm done for today. Here are some of the remaining points to cover (mainly leaving here for myself):

• Check if property paths will work in the get of the relational properties.
• Go through the relevant tests, see if everything is covered in regards to objects/arrays as values.
• Discuss and agree on the arbitrary object vs internal complex value (relations) topic.
• Modify the ModelDefinition type to support nested objects in TypeScript. This relates to both creating and querying models/entities.

[gidesan]

@gidesan, I'd love to hear your feedback on this. Please do not proceed with implementation until we discuss this in sufficient detail.

Not a strong opinion on that. I'd say option 2 is cleaner, but I also guess collisions for option 1 will be very rare (and they can be notified to the user). Available for help if needed ofc :)

[kettanaito]

Primary key and Relations as classes

I've rewritten the internal implementation of the primary key and relations and those are now represented as classes (see my post above for the motivation to do so). No user-facing changes were introduced, apart from a new added invariant:

• When producing (resolving) relation for a non-existing model, or existing model that has no primary key (unlikely, would fail on the parsing stage), an exception will be thrown.

[kettanaito]

I've spotted that removeInternalProperties does not support nested relations. This surfaces when querying through nested relations, for which we don't have tests either. I'll add such tests.

[kettanaito]

Added a test for updating a nested property of an entity (no relations).

The only thing that remains now is the proper TypeScript support for deeply nested objects. This may be a challenge.

[kettanaito]

I've spent some time adjusting the type definitions to support nested objects. Boy, is that a challenge. It comes down to enabling nested objects type-wise for the following areas:

• Creating a model (initial values).
• Querying a model (findFirst/updateMany/etc.).
• Sorting through model's properties (orderBy in updateMany).
• Updating a model (must suggest existing properties, including nested ones).

I'm more or less done with everything, covering up some collateral issues. @gidesan, please don't push any substantial changes, especially type-related ones. My mind will explode if I have to rebase what I have. Thanks!

[kettanaito]

Everything should be in order now: the feature is implemented, typed, relevant tests updated/added.

Thank you for the great work on this, @gidesan! 🎉

[kettanaito]

There's another issue I've found with update and the derived next properties (value-as-a-function). The entire updateEntity function doesn't seem to support nested objects. Working on a fix before the next release.