Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

omit entity properties into usecase #49

Closed
italojs opened this issue Sep 27, 2021 · 19 comments
Closed

omit entity properties into usecase #49

italojs opened this issue Sep 27, 2021 · 19 comments
Assignees
Labels
enhancement New feature or request hacktoberfest help wanted Extra attention is needed severity-nice-to-have Item is nice to have wip Work in progress

Comments

@italojs
Copy link
Member

italojs commented Sep 27, 2021

I would like to have a simlilar feature like ZOD omit feature

The idea is to avoid create multi entities when I need a valueobject for my usecase, e.g

const User = entitity('User', {
id: field(string),
name: field(string)
}

usecase(
    'create user',
    {
      request: User, // I dont want to receive id here
    [...] 

My propose is:

const User = entitity('User', {
id: field(string),
name: field(string)
}

// option #1
usecase(
    'create user',
    {
      request: User.toValueObject() // omit default values: id, createdAt, deleteAt, updatedAt
    [...] 
// option #2
// we can merge it with option 1
usecase(
    'create user',
    {
      request: User.omit([ 'id', 'createdAt', 'myOtherProperty']) 
    [...] 
// option #3
const User = entitity('User', {
id: field(string, { omitable: true } ),
myCustomProp: field(Number, { omitable: true }
name: field(string)
}

usecase(
    'create user',
    {
      request: User.toValueObject() // Omit 'id' and  'myCustomProp'
    [...] 
@italojs italojs added the enhancement New feature or request label Sep 27, 2021
@jhomarolo
Copy link
Contributor

jhomarolo commented Sep 27, 2021

I like the idea of a mix a little bit your sugestions:

  1. Have the myCustomProp: field(Number, { omitable: true } in the entity
  2. The entity that have omitable = true, wont be exported in the toValueObject() method. By default the method toValueObject() wont supress any field, unless they have the omitable = true.
  3. Also have the omit() method to explicit wich field you don't want to export.

What do you think?

@dalssoft
Copy link
Member

If the goal is to have a request object, there is this issue for the same goal but with a different approach: herbsjs/buchu#53

@italojs
Copy link
Member Author

italojs commented Oct 25, 2021

@dalssoft I think when we use the herbsjs/buchu#53 approach, we create a strong coupling between the glues, I think if we implements a omitable fields, we could have a generic feature that can be used in many scenarios.

I liked the @jhomarolo suggestion

@dalssoft
Copy link
Member

we create a strong coupling between the glues

How so?

IMO, the suggestions on this issue creates a undesired coupling Entity -> Usecase by creating features on gotu to satisfy needs in buchu. Remember: Use cases (buchu) can/should know about Entities (gotu) but the reverse is not true.

We should invert this dependency (Usecase -> Entity), as suggested in herbsjs/buchu#53 , so a usecase can read metadata from a entity.

BTW, it would be ok to create a feature in gotu if it would serve other scenarios related to entities. But looks like .omit, omitable and toValueObject serves just for the request scenario.

@jhomarolo jhomarolo added blocked Something is blocked help wanted Extra attention is needed severity-nice-to-have Item is nice to have labels Dec 24, 2021
@italojs italojs closed this as completed Mar 22, 2022
@italojs italojs reopened this Mar 22, 2022
@italojs
Copy link
Member Author

italojs commented Mar 22, 2022

I think we could to starting just avoiding the ID

const User = entity('User',{
  id: id(Number),
  name: field(String),
  password: field(String)
}
[...]
User.toValueObject()
/* return new entity
nickName
password
*/

@italojs italojs added wip Work in progress and removed blocked Something is blocked labels Mar 22, 2022
@jhomarolo
Copy link
Contributor

@italojs do you think to start with toValueObject() without using the field(Number, { omitable: true } ?

@dalssoft
Copy link
Member

  1. If this is not related to usecase request, whats the use case here?
  2. Is it related to thie PR feat(base-entity): fields and ids getters on base entity #57 ?

@italojs
Copy link
Member Author

italojs commented Mar 23, 2022

i guess the #57 PR is only for metadata, not a feature

@italojs
Copy link
Member Author

italojs commented Mar 23, 2022

@jhomarolo I implemented your suggestion #59

@dalssoft
Copy link
Member

Guys, I don't recommend putting energy on this feature until we have a clear use case for it

@italojs
Copy link
Member Author

italojs commented Mar 24, 2022

beside the use in usecase, that will impact the herbs2Gql and herbs2rest, sometime we need to remove the presence: true of some property only to validade the received data. e.g.

const User = entity('User', {
   id: id(Number, { presence: true }),
   name: field(String, { presence: true })),
   email: field(String, { presence: true, validation: { email: true }  })),
   createdAt: field(Date, { presence: true }))
})
 
[...] 

request: {
  name: String,
  email: String
}

[...]
'validate the received data': step( ctx => {
        const user = User.fromJSON(ctx.req)
        if(!user.isValid())  { 
             // I can't do this validation becase the id and createdAt is required
             // to do it works, today I need to remove the "presence: true" from fields id and createdAt, it's not a good ideia
        }
}

with the omitable fields I avoid this problem:

request: User.asValueObject()
[...]
'validate the received data': step( ctx => {
        const user = User.fromJSON(ctx.req)
        if(!user.isValid({ avoidOmitableFields: true })){ 
        }
}

@italojs
Copy link
Member Author

italojs commented Apr 14, 2022

@dalssoft

@dalssoft
Copy link
Member

for this use case I can think in several approaches:

(1) Check for error return

Without extra implementation, it would be possible to check the returned error code:

if(!user.isValid()){ 
	if (user.error...) {
		// check if the only error came from `createdAt`, regarding `presence`.
		// we could create a helper function to do that
	}
}

(2) Validation Ignore or Override

if(!user.isValid({ ignore: { createdAt: "presence"} })){ 

or

if(!user.isValid({ override: { createdAt: { presence: false } } })){ 

with that there is no need to have any extra metadata on the entity (as suggested: { omitable: true }) since omitable depends on the context, which takes us to the next approuch.

(3) Validation Contexts

Their could have be a extra metadata on the entity for validation scenarios, like:

const User = entity('User', {
	id: id(Number, { presence: true }),
	name: field(String, { presence: true })),
	email: field(String, { presence: true, validation: { email: true }  })),
	createdAt: field(Date, { presence: true }))

	create: validation({
		{ override: { createdAt: { presence: false } } }
	}),
})

and

if(!user.isValid({ context: 'create' })){ 

@italojs
Copy link
Member Author

italojs commented May 5, 2022

approach (1) We cant check only the presence: true fields because I could to omit a required field, the createdAt is a good example, it's mandatory but I don't need this field when I'm creating the entity

approach (2)
It's not a good DX because I will need aways to specify my fields and when I have maaany fields to ignore, I will create an ugly code

approach (3)
it will generate so much code and the DX will decrease, I prefer to implement this omitable function in a utils folder at a unique time then write many validation functions for many scenarios.

This feature is useful, it's so necessary that exists a lib to do it (lodash.omit) and the typescript implements it as a native feature as well, we cant imagine when the developer will use it and it must to be easy to use.

@dalssoft
Copy link
Member

@italojs lodash's omit is a object "clone" method, not a validation mechanism. not related to the use case you brought

@VictorTBX
Copy link
Contributor

I'm working on this issue through herbsjs/buchu#53

@italojs
Copy link
Member Author

italojs commented Jun 7, 2022

@VictorTBX already have an pr for this #59

@VictorTBX
Copy link
Contributor

@italojs I'm following @dalssoft idea commented on herbsjs/buchu#53

Using the approach: request: request.from(Item, { ignore: ['createdAt'] }),

@dalssoft
Copy link
Member

dalssoft commented Jan 3, 2023

This issue started as a feature request to improve converting a entity to a request object with the possibility to remove some fields.

This was implemented here: herbsjs/buchu#53

After some discussion, other request appeared: the option to have some partial or custom validation for a entity.

In order to keep the discussion focused, I've created a new issue to discuss this: #70

Given that, I'm closing this issue here and we can continue the discussion in the new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest help wanted Extra attention is needed severity-nice-to-have Item is nice to have wip Work in progress
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants