herbsjs / herbs-cli

Herbs CLI
https://herbsjs.org/
MIT License
29 stars 30 forks source link

feat: including relationships in migrations #144

Open endersoncosta opened 2 years ago

endersoncosta commented 2 years ago

With this we can write the relationship migrations based on our domain entities.

jhomarolo commented 2 years ago

@endersoncosta the ci is breaking: https://github.com/herbsjs/herbs-cli/runs/7196619032?check_suite_focus=true

endersoncosta commented 2 years ago

Locally the tests are passing through. And other PRs apparently has the same problem from time to time.

Is possible to re-run the CI workflow?

codecov[bot] commented 2 years ago

Codecov Report

Merging #144 (fb0b418) into main (56e99b7) will decrease coverage by 0.15%. The diff coverage is 78.57%.

:exclamation: Current head fb0b418 differs from pull request most recent head d8a12c6. Consider uploading reports for the commit d8a12c6 to get more accurate results

@@            Coverage Diff             @@
##             main     #144      +/-   ##
==========================================
- Coverage   86.28%   86.13%   -0.16%     
==========================================
  Files          25       25              
  Lines         423      440      +17     
==========================================
+ Hits          365      379      +14     
- Misses         58       61       +3     
Impacted Files Coverage Δ
...rators/src/infra/database/migrations/migrations.js 88.46% <73.91%> (-3.65%) :arrow_down:
src/generators/utils.js 83.33% <100.00%> (+1.85%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us.

dalssoft commented 2 years ago

@endersoncosta about this generated code:

// table.integer('userId').unsigned().nullable()
// table.foreign('userId').references('id').inTable('users')

(1) why .unsigned()? (2) why .nullable()? (3) why commented code? the dev would have other options for this operation? (4) just table.integer('userId').references('id').inTable('users') works?

about this one:

knex.schema.table('markets', function (table) {
    table.integer('companyId').unsigned().index().references('id').inTable('companys')
})

(5) why .index()? FKs are normally already indexed.

endersoncosta commented 2 years ago

@endersoncosta about this generated code:

// table.integer('userId').unsigned().nullable()
// table.foreign('userId').references('id').inTable('users')

(1) why .unsigned()? (2) why .nullable()? (3) why commented code? the dev would have other options for this operation?
(4) just table.integer('userId').references('id').inTable('users') works?

about this one:

knex.schema.table('markets', function (table) {
    table.integer('companyId').unsigned().index().references('id').inTable('companys')
})

(5) why .index()? FKs are normally already indexed.

1.unsigned() indeed this is used in MySQL to define the column to has just positive numbers, I think it is a good practice, but doesn't make any sense in postgres.

  1. To the initial setup this really doesn't has reason, but when we change the structure of an existing table, this can be necessary...

  2. We can't garantee the execution order, if the dev run all the migrations at the same time, this will break the execution with errors.

  3. Indeed, it is better in that simpler way.

  4. Indeed.

dalssoft commented 2 years ago

the CLI created the default entity user.js:

const User =
        entity('User', {
          id: id(String),
          ...
        })

when adding an entity with a reference:

const Project =
    entity('Project', {
        id: id(String),
        user: field(User),
    })

the generated migration:

    .createTable('projects', function (table) {
        table.string('id').primary()   
        table.integer('user_id').references('id').inTable('users')

here the user_id should be .string() to follow the same type as the entity.

the same is true for 1:many:

    .table('users', function (table) {
        table.integer('customer_id').references('id').inTable('customers')
    })