feat: remove all internals from API model

[marcosvega91]

In this PR I have removed all the internals from API.
To do so I have removed remove coupling of api with itself and add the map transformation on different method returns.

I have also removed the map from the rest handlers because it is already done in the api.

I have created a new Entity type, the public one, and replaced the old one with InternalEntity.

GitHub

• Closes Omit internal properties when returning a public record #41

Changes

• Separates entities in Entity and InternalEntity.
• Adds an enum type for internal entity properties (__type, __primaryKey, etc.).
• Removes internal entity properties from public entities (i.e. from db[model].create() return payload).
• Sets primaryKey on the Relation when it's defined by looking it up in the database for the referenced model.
• Resolves relational properties against the Relation with no internal properties on the actual value (no longer needed, relation contains all the necessary info).
• Makes defined relational object properties enumerable (Error when calling findFirst even though nothing has changed #78).

[marcosvega91]

We can assume that primary key exists because we have already checked it when the factory is first created

[kettanaito]

That's a good point. I'd rather parse primaryKey in parseModelDeclaration and throw away findPrimaryKey function. It's confusing why the model that's known is parsed only when an entity is created. Needless to say, that creating new entities doesn't change model declaration's properties or relations. Looks like a design overhaul to me, and I'd like to address it in the follow-up pull request.

[kettanaito]

Hey, @marcosvega91. Thank you for shipping this! I've rebased the feature branch to include the latest which -> where change. I will read through the changes, I'm curious how relations properties now understand that type of node value is being passed to them if we no longer expose its type publicly. A great chance for me to learn.

[kettanaito]

This issue is more complicated than it appears. Since we've removed the internal properties from the entity, they are no longer present in the entity instance returned from the model API methods (create/update/etc.):

const usa = db.country.create({ name: 'USA' })

This is intended, as we wish for the public entity not to contain internal things. However, this means that relational properties cannot validate what kind of data you pass into them. Consider this:

const db = factory({
  country: { id: primaryKey(String), name: String },
  capital: { id: primaryKey(String), country: oneOf('country') }
})

const usa = db.country.create({ name: 'USA' })

const notCountry = {
  id: 'abc-123',
  name: 'Made up'
}

// This is OK, as we pass an entity as the reltional value.
db.capital.create({ country: usa })

// However, this is also OK because a non-entity object "notCountry"
// is compatible with its properties, although it's not the correct value to pass
// as it won't get resolved (doesn't have "__type", "__primaryKey", and "__nodeId")
// to look up the relational reference. 
db.capital.create({ country: notCountry })

I've added this typings test to test/typings.ts that will now fail since we're breaking a type validation expectation.

I think we may design the internal logic in a different way, as all the necessary info for the relational lookup is known at the model declaration time:

{
  country: {
    capital: manyOf('capital')
  }
}

Knowing that the referenced model is "capital" (_type), we can get its primary key. Then whenever you pass a value to the country.capital, we can expect that value to have a primary key and do a lookup only based on the value (type and primary key name known at the declaration time).

@marcosvega91, what do you think about this? Was I successful in explaining the problem?

[marcosvega91]

ah yes, I didn't think about it. When we'll create a new entity we should verify the existence of all relational entities

[kettanaito]

@marcosvega91, yeah. I'll separate the concerns and make the relational properties work based on the model definition, without us having to return the data like referenced entity __type and __nodeId (those are known when the property is defined). This will be addressed in #72.

[marcosvega91]

Hey @kettanaito I think I have dropped one of your commit here. The one that you have pushed with the refactor of removeInternalProperties util. Could I ask you to cherry pick from your local ?

I have rebase the branch with the current master

[kettanaito]

@marcosvega91, no worries. I've applied that refactoring on top of your feature branch.

[kettanaito]

Could you please tell me more about how you replace the entity references in relational properties?

Previously, the definition of the relational property produces a short Relation object:

factory({
  user: { country: oneOf('country') }
})

user.country // { kind: 'OneOf', modelName: 'country', unique: false }

Then, in defineRelationalProperties, when the actual entity values are known, they were used as an example of what that relation is:

data/src/model/defineRelationalProperties.ts

Lines 31 to 34 in 39c078b

	// Take the relational entity reference from the initial values.
	const entityRefs: EntityInstance<any, any>[] = [].concat(
	initialValues[property],
	)

For instance,

db.user.create({
  country: db.country.create()
  // country: { __type: "country", __primaryKey: "id", ...initialValues }
})

With the removal of the internal entity properties like __type and __primaryKey, the latter won't be set on the actual value (won't be returned from db[model].create). I'd like to learn more about your thinking to tackle this. Thanks!

[marcosvega91]

I have added the primary key in the Relation type. In this way when a new relation entity is created we already have all we need to reference it. The __type was already present as modelName

[kettanaito]

Awesome! Got it now, thanks for the details.

[kettanaito]

A great improvement. Thank you, @marcosvega91!

[marcosvega91]

This can be useful also for the issue #56, we'll have a better access to all the things we need to generate rest apis using relations

[kettanaito]

True! I'm glad this touched relations a little, it will indeed give us more control over them in the future, as all the necessary data to resolve a relation is succinctly available on the Relation node.