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

Automatically identify pk and fks into repository layer #23

Open
italojs opened this issue Sep 14, 2021 · 7 comments
Open

Automatically identify pk and fks into repository layer #23

italojs opened this issue Sep 14, 2021 · 7 comments
Labels
enhancement New feature or request hacktoberfest help wanted Extra attention is needed severity-minor Item is not urgent

Comments

@italojs
Copy link
Member

italojs commented Sep 14, 2021

Following the issue herbsjs/herbs-cli#26

I would like to have an automatic PK and FK identification for my herbs2knex repositories

e.g:

USERS_TABLE PRODUCTS_TABLESELES_TABLE
id name email
12 italo [email protected]
13 jhow [email protected]
14 david [email protected]
id name price
3 mouse 50
4 monitor 200
5 keyboard 40
id userId productId
34 12 3
35 12 5
36 13 3

Today we have to explicity inform what is my PK and my FK with ids and foreignKeys keys

entity("Sale", {
    id: field(Number),
    product: field(Product),
    user: field(User), 
})
// ----------------------------------------
class SaleRepository extends Repository {    
   constructor() {       
       super({            
         entity: Sale,            
         table: 'SALES_TABLE',            
         ids: ['id'], 
         foreignKeys: [{ userId: Number, productId: Number }], 
         knex: connection        
       })   
     }
}

Instead write foreignKeys: [{ userId: Number, productId: Number }] and ids: ['id'] I would like the herbs2knex identify my FKs and Pks automagically.

My sugestion

  • When we don't specify our ID/PK the herbs2knex will look for id property into father entity(Sale)
  • When we don't specify our FKs the herbs2knex will look each father's property and for each entity will look for an id property
class SaleRepository extends Repository {    
   constructor() {       
       super({            
         entity: Sale,            
         table: 'SALES_TABLE', 
         knex: connection        
       })   
     }

If we need to have complex fields like :

entity("User", {
    id: field(Number),
    [...]
    address: field(Address)
})

we can check if this have an ID property, if doesn't we just don't process it as an FK.
it makes match with herbsjs/gotu#46

future possibilities

When we identify a entity that have relations inside, we can make "lazy load/insert" into repositories too, example:

entity("Sale", {
    id: field(Number),
    product: field(Product),
    user: field(User), 
})
// ---------------------------------
const entity = saleRepository.FindByID(123)
/*
id: 123
product: {
    id: 23,
    name: 'mouse',
    price: 123.23
},
user: {
   id: 155,
   name: 'italo',
   email: '[email protected]'
}
*/
@italojs italojs added the enhancement New feature or request label Sep 14, 2021
@dalssoft
Copy link
Member

What if we had something like this?

class SaleRepository extends Repository {    
   constructor() {       
       super({            
         entity: Sale,            
         table: 'SALES_TABLE',            
         ids: ['id'], 
         foreignKeys: [{ userId: Number, productId: Number }], 
         knex: connection        
       })

       findByUser(userId) {
         const userRepo = new UserRepository()
         this.join(userRepo, { fk: 'userId' }).findByID(userID)
       }
     }
}

@italojs
Copy link
Member Author

italojs commented Sep 21, 2021

talking about the query methods, I think we can have a FindByFK(fk, id) method inside Repository class, e.g:

class Repository {
  constructor(options) { [...] }

   async findByFK(fk, values) {
       const fks = this.dataMapper.tableFKs()
       const tableFields = this.dataMapper.tableFields()

       const parsedValue = Array.isArray(values) ? values : [values]
       const ret = await this.runner()
         .select(tableFields)
         .whereIn(fks[fk], parsedValue)

       const entities = []

       for (const row of ret) {
         if (row === undefined) continue
         entities.push(this.dataMapper.toEntity(row))
       }
      return entities
  }

the usage will be something like this

cons salesRepo = new SaleRepository(...)
salesRepo.findByFK('userId', 12321)

but I think it's not the central point, the problem here is explicitly configure the FKs, bacause it's a problem to herbs-cli today ( herbsjs/herbs-cli#26 )

@italojsv
Copy link

italojsv commented Oct 4, 2021

@dalssoft

@dalssoft
Copy link
Member

findByUser(userId) could be a generated alias method to .findByFK('userId', userId).

but in order to fetch an User entity properly SaleRepository should know UserRepository, which is the repository who knows about User table and its schema.

that is the reason why I suggest having a .join method between repositories. that is just a brainstorm, but the point is SaleRepository shouldn't know much about User table and if it want to know, it should as UserRepository.

@jhomarolo jhomarolo added help wanted Extra attention is needed severity-minor Item is not urgent labels Dec 24, 2021
@italojs
Copy link
Member Author

italojs commented Apr 14, 2022

I think we change the original issue focus, the main problem is too need to explicit specify the fks when we can do it automagically.

What we have to do today:

const Address = entity("Address", {
   addressID: id(Number),
   [...]
})

entity("User", {
    userID: id(Number),
    [...]
    addressID: field(Number) // we cant use nested entities
})

const Sale = entity("Sale", {
 saleID: field(Number), 
 userID: field(Number),// we cant use nested entities
 [...] 
}

so, to have the 3 entities at same time/step/usecase i need to do 3 queries

const sale = salesRepo.findByID(99)
const user = userRepo.findByID(sale.userID)
const address = addressRepo.findByID(user.addressID)

instead to do 3 queries, I would to to only 1 query and it brings for me the all nested entities

const Address = entity("Address", {
   addressID: id(Number),
   [...]
})

entity("User", {
    userID: id(Number),
    [...]
    address: field(Address)
})

const Sale = entity("Sale", {
 saleID: field(Number), 
 user: field(User),
 [...] 
}

class SaleRepository extends Repository {    
   constructor() {       
       super({            
         entity: Sale,
         // I dont specify the fks userID or AndressID, the herbs2knex do it for me.           
         table: 'SALES_TABLE', 
         knex: connection        
       })   
     }


const saleRepo = new SaleRepository([...])
const sale = saleRepo.finByID(99)
/*
sale:
id: 99
user: {
   id: 155,
   address: {
         id: 88
         [...]
}
*/

in the database I have 3 tables, but into the code I have "1" entity

@dalssoft
Copy link
Member

I think we change the original issue focus, the main problem is too need to explicit specify the fks when we can do it automagically.

I think we are still on the same page: how to fetch data from associates entities.

But first, you should not model your entities like that. The example you gave is a 1:1 table translation to entity. The problem with that is that it no longer follows a OO approach.

My suggestion for this model would be:

entity("Address", {
   ID: id(Number),
   [...]
})

entity("User", {
    ID: id(Number),
    [...]
    address: field(Address)
})

const Sale = entity("Sale", {
 ID: id(Number), 
 user: field(User)
 [...] 
}

It is not a small thing. Entities are OO and we should start there (from domain to infra). We should leave to the repositories the hard work of translating OO to entity–relationship model (for relational database) by reading the metadata from the entities and by the extra metadata we provide direct to the repository (Ex: table name, FKs, etc).

I dont specify the fks userID or AndressID, the herbs2knex do it for me.

It can't do it without extra metadata. Due to OO / Relational model impedance (1 2 3), translating a object association into a FK is not straightforward.

My suggestion is that the missing extra metadata is provided at the repository level, as posted before (#23 (comment)):

class SaleRepository extends Repository {    
   constructor() {       
       super({            
         entity: Sale,            
         table: 'SALES',            
         ids: ['id'], // optional with the ids fields from the entity
         foreignKeys: [ { user: { field: 'userId', entity: User, repository: UserRepository } } ], 
         knex: connection        
       })
     }
}

With this extra info the repository can (1) create methods for this relationship (ex: findByUser as post here #23 (comment)) and (2) be able to communicate with other repositories to know how to retrieve data for that entity.

And with that things like that would be possible:

const saleRepo = new SaleRepository([...])
const sale = saleRepo.finByID(99, { fetch: { User: { Address: true } } )  // explicit fetch
/*
sale:
id: 99
user: {
   id: 155,
   address: {
         id: 88
         [...]
}
*/

Here the SaleRepository could use two approaches / strategies:

(1) fetch data from SALES table, get the user IDs and now ask for UserRepository for users infos. This would be the multiple queries approach. For instance, Rails and similars do that most of the time.

(2) try to be smart and create a single query to fetch all the data. That would be my last option, since we are trying to avoid being a ORM and trying to keep the things simple, given that this approach can get ugly / complex very very fast.

@dalssoft dalssoft moved this to More discussion is needed in Herbs - 3rd Anniversary Edition May 25, 2022
@jhomarolo jhomarolo changed the title Automatclly identify pk and fks into repository layer Automatcally identify pk and fks into repository layer Jun 7, 2022
@jhomarolo jhomarolo changed the title Automatcally identify pk and fks into repository layer Automatically identify pk and fks into repository layer Jun 7, 2022
@danielsous
Copy link

danielsous commented Jul 27, 2022

Hello everyone, I added this automatic generation of primary key and foreign key in Herbs Update CLI, I believe this will make it easier to solve this improvement in herbs2knex

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-minor Item is not urgent
Projects
Status: Ready to code
Development

No branches or pull requests

5 participants