feat: including relationships in migrations by endersoncosta · Pull Request #144 · herbsjs/herbs-cli

endersoncosta · 2022-07-05T12:52:50Z

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

jhomarolo · 2022-07-05T13:00:31Z

+ <%if (props.ref) { 
+ props.ref.forEach(function(link){ 
+ if (link.relationship === 'One-to-One') { %>
+ // table.integer('<%-link.columnName %>').unsigned().nullable()


this comment is necessary?

the idea are to not create a migration that does not work, due to not has an order of migrations when we write the files. These relationships throw errors due the dependence dont exist yet ...

jhomarolo · 2022-07-05T13:02:11Z

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

endersoncosta · 2022-07-06T12:30:58Z

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 · 2022-07-07T12:20:57Z

Codecov Report

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

❗ 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%)`	⬇️
src/generators/utils.js	`83.33% <100.00%> (+1.85%)`	⬆️

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

dalssoft · 2022-07-08T01:50:27Z

@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.

dalssoft · 2022-07-08T03:04:24Z

 template: `infra/data/database/${db.toLowerCase()}/migration.ejs`,
 target: migrationFullPath,
- props: { table: `${camelCase(name)}s`, columns: columns }
+ props: { table: `${camelCase(name)}s`, externalColumnName: `${camelCase(name)}Id`, columns, ref, idColumn }


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:36Z

+ }
+
+ if (isArrayWithType(type) && herbs.entity.isEntity(type[0]))
+ refs.push({ table: `${camelCase(type[0].name)}s`, relationship: 'One-to-Many' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:52Z

+ if (herbs.entity.isEntity(type)) {
+ const typeSchema = type.prototype.meta.schema
+ const idRef = Object.values(typeSchema).find(column => column.options.isId)?.name
+ refs.push({ id: idRef, columnName: `${camelCase(name)}Id`, table: `${camelCase(type.name)}s`, relationship: 'One-to-One' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

endersoncosta · 2022-07-25T12:51:07Z

@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.

To the initial setup this really doesn't has reason, but when we change the structure of an existing table, this can be necessary...
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.
Indeed, it is better in that simpler way.
Indeed.

pull main

dalssoft · 2022-07-27T23:40:33Z

the CLI created the default entity user.js:

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

when adding an entity with a reference:

constProject=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')})

endersoncosta · 2022-07-05T12:52:50Z

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

jhomarolo · 2022-07-05T13:00:31Z

+ <%if (props.ref) { 
+ props.ref.forEach(function(link){ 
+ if (link.relationship === 'One-to-One') { %>
+ // table.integer('<%-link.columnName %>').unsigned().nullable()


this comment is necessary?

the idea are to not create a migration that does not work, due to not has an order of migrations when we write the files. These relationships throw errors due the dependence dont exist yet ...

jhomarolo · 2022-07-05T13:02:11Z

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

endersoncosta · 2022-07-06T12:30:58Z

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 · 2022-07-07T12:20:57Z

Codecov Report

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

❗ 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%)`	⬇️
src/generators/utils.js	`83.33% <100.00%> (+1.85%)`	⬆️

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

dalssoft · 2022-07-08T01:50:27Z

@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.

dalssoft · 2022-07-08T03:04:24Z

 template: `infra/data/database/${db.toLowerCase()}/migration.ejs`,
 target: migrationFullPath,
- props: { table: `${camelCase(name)}s`, columns: columns }
+ props: { table: `${camelCase(name)}s`, externalColumnName: `${camelCase(name)}Id`, columns, ref, idColumn }


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:36Z

+ }
+
+ if (isArrayWithType(type) && herbs.entity.isEntity(type[0]))
+ refs.push({ table: `${camelCase(type[0].name)}s`, relationship: 'One-to-Many' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:52Z

+ if (herbs.entity.isEntity(type)) {
+ const typeSchema = type.prototype.meta.schema
+ const idRef = Object.values(typeSchema).find(column => column.options.isId)?.name
+ refs.push({ id: idRef, columnName: `${camelCase(name)}Id`, table: `${camelCase(type.name)}s`, relationship: 'One-to-One' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

endersoncosta · 2022-07-25T12:51:07Z

@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.

To the initial setup this really doesn't has reason, but when we change the structure of an existing table, this can be necessary...
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.
Indeed, it is better in that simpler way.
Indeed.

pull main

dalssoft · 2022-07-27T23:40:33Z

the CLI created the default entity user.js:

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

when adding an entity with a reference:

constProject=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')})

endersoncosta · 2022-07-05T12:52:50Z

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

jhomarolo · 2022-07-05T13:00:31Z

+ <%if (props.ref) { 
+ props.ref.forEach(function(link){ 
+ if (link.relationship === 'One-to-One') { %>
+ // table.integer('<%-link.columnName %>').unsigned().nullable()


this comment is necessary?

the idea are to not create a migration that does not work, due to not has an order of migrations when we write the files. These relationships throw errors due the dependence dont exist yet ...

jhomarolo · 2022-07-05T13:02:11Z

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

endersoncosta · 2022-07-06T12:30:58Z

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 · 2022-07-07T12:20:57Z

Codecov Report

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

❗ 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%)`	⬇️
src/generators/utils.js	`83.33% <100.00%> (+1.85%)`	⬆️

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

dalssoft · 2022-07-08T01:50:27Z

@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.

dalssoft · 2022-07-08T03:04:24Z

 template: `infra/data/database/${db.toLowerCase()}/migration.ejs`,
 target: migrationFullPath,
- props: { table: `${camelCase(name)}s`, columns: columns }
+ props: { table: `${camelCase(name)}s`, externalColumnName: `${camelCase(name)}Id`, columns, ref, idColumn }


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:36Z

+ }
+
+ if (isArrayWithType(type) && herbs.entity.isEntity(type[0]))
+ refs.push({ table: `${camelCase(type[0].name)}s`, relationship: 'One-to-Many' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:52Z

+ if (herbs.entity.isEntity(type)) {
+ const typeSchema = type.prototype.meta.schema
+ const idRef = Object.values(typeSchema).find(column => column.options.isId)?.name
+ refs.push({ id: idRef, columnName: `${camelCase(name)}Id`, table: `${camelCase(type.name)}s`, relationship: 'One-to-One' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

endersoncosta · 2022-07-25T12:51:07Z

@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.

To the initial setup this really doesn't has reason, but when we change the structure of an existing table, this can be necessary...
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.
Indeed, it is better in that simpler way.
Indeed.

pull main

dalssoft · 2022-07-27T23:40:33Z

the CLI created the default entity user.js:

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

when adding an entity with a reference:

constProject=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')})

endersoncosta · 2022-07-05T12:52:50Z

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

jhomarolo · 2022-07-05T13:00:31Z

+ <%if (props.ref) { 
+ props.ref.forEach(function(link){ 
+ if (link.relationship === 'One-to-One') { %>
+ // table.integer('<%-link.columnName %>').unsigned().nullable()


this comment is necessary?

the idea are to not create a migration that does not work, due to not has an order of migrations when we write the files. These relationships throw errors due the dependence dont exist yet ...

jhomarolo · 2022-07-05T13:02:11Z

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

endersoncosta · 2022-07-06T12:30:58Z

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 · 2022-07-07T12:20:57Z

Codecov Report

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

❗ 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%)`	⬇️
src/generators/utils.js	`83.33% <100.00%> (+1.85%)`	⬆️

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

dalssoft · 2022-07-08T01:50:27Z

@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.

dalssoft · 2022-07-08T03:04:24Z

 template: `infra/data/database/${db.toLowerCase()}/migration.ejs`,
 target: migrationFullPath,
- props: { table: `${camelCase(name)}s`, columns: columns }
+ props: { table: `${camelCase(name)}s`, externalColumnName: `${camelCase(name)}Id`, columns, ref, idColumn }


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:36Z

+ }
+
+ if (isArrayWithType(type) && herbs.entity.isEntity(type[0]))
+ refs.push({ table: `${camelCase(type[0].name)}s`, relationship: 'One-to-Many' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:52Z

+ if (herbs.entity.isEntity(type)) {
+ const typeSchema = type.prototype.meta.schema
+ const idRef = Object.values(typeSchema).find(column => column.options.isId)?.name
+ refs.push({ id: idRef, columnName: `${camelCase(name)}Id`, table: `${camelCase(type.name)}s`, relationship: 'One-to-One' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

endersoncosta · 2022-07-25T12:51:07Z

@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.

To the initial setup this really doesn't has reason, but when we change the structure of an existing table, this can be necessary...
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.
Indeed, it is better in that simpler way.
Indeed.

pull main

dalssoft · 2022-07-27T23:40:33Z

the CLI created the default entity user.js:

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

when adding an entity with a reference:

constProject=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')})

endersoncosta · 2022-07-05T12:52:50Z

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

jhomarolo · 2022-07-05T13:00:31Z

+ <%if (props.ref) { 
+ props.ref.forEach(function(link){ 
+ if (link.relationship === 'One-to-One') { %>
+ // table.integer('<%-link.columnName %>').unsigned().nullable()


this comment is necessary?

the idea are to not create a migration that does not work, due to not has an order of migrations when we write the files. These relationships throw errors due the dependence dont exist yet ...

jhomarolo · 2022-07-05T13:02:11Z

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

endersoncosta · 2022-07-06T12:30:58Z

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 · 2022-07-07T12:20:57Z

Codecov Report

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

❗ 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%)`	⬇️
src/generators/utils.js	`83.33% <100.00%> (+1.85%)`	⬆️

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

dalssoft · 2022-07-08T01:50:27Z

@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.

dalssoft · 2022-07-08T03:04:24Z

 template: `infra/data/database/${db.toLowerCase()}/migration.ejs`,
 target: migrationFullPath,
- props: { table: `${camelCase(name)}s`, columns: columns }
+ props: { table: `${camelCase(name)}s`, externalColumnName: `${camelCase(name)}Id`, columns, ref, idColumn }


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:36Z

+ }
+
+ if (isArrayWithType(type) && herbs.entity.isEntity(type[0]))
+ refs.push({ table: `${camelCase(type[0].name)}s`, relationship: 'One-to-Many' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:52Z

+ if (herbs.entity.isEntity(type)) {
+ const typeSchema = type.prototype.meta.schema
+ const idRef = Object.values(typeSchema).find(column => column.options.isId)?.name
+ refs.push({ id: idRef, columnName: `${camelCase(name)}Id`, table: `${camelCase(type.name)}s`, relationship: 'One-to-One' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

endersoncosta · 2022-07-25T12:51:07Z

@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.

To the initial setup this really doesn't has reason, but when we change the structure of an existing table, this can be necessary...
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.
Indeed, it is better in that simpler way.
Indeed.

pull main

dalssoft · 2022-07-27T23:40:33Z

the CLI created the default entity user.js:

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

when adding an entity with a reference:

constProject=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')})

endersoncosta · 2022-07-05T12:52:50Z

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

jhomarolo · 2022-07-05T13:00:31Z

+ <%if (props.ref) { 
+ props.ref.forEach(function(link){ 
+ if (link.relationship === 'One-to-One') { %>
+ // table.integer('<%-link.columnName %>').unsigned().nullable()


this comment is necessary?

the idea are to not create a migration that does not work, due to not has an order of migrations when we write the files. These relationships throw errors due the dependence dont exist yet ...

jhomarolo · 2022-07-05T13:02:11Z

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

endersoncosta · 2022-07-06T12:30:58Z

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 · 2022-07-07T12:20:57Z

Codecov Report

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

❗ 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%)`	⬇️
src/generators/utils.js	`83.33% <100.00%> (+1.85%)`	⬆️

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

dalssoft · 2022-07-08T01:50:27Z

@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.

dalssoft · 2022-07-08T03:04:24Z

 template: `infra/data/database/${db.toLowerCase()}/migration.ejs`,
 target: migrationFullPath,
- props: { table: `${camelCase(name)}s`, columns: columns }
+ props: { table: `${camelCase(name)}s`, externalColumnName: `${camelCase(name)}Id`, columns, ref, idColumn }


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:36Z

+ }
+
+ if (isArrayWithType(type) && herbs.entity.isEntity(type[0]))
+ refs.push({ table: `${camelCase(type[0].name)}s`, relationship: 'One-to-Many' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:52Z

+ if (herbs.entity.isEntity(type)) {
+ const typeSchema = type.prototype.meta.schema
+ const idRef = Object.values(typeSchema).find(column => column.options.isId)?.name
+ refs.push({ id: idRef, columnName: `${camelCase(name)}Id`, table: `${camelCase(type.name)}s`, relationship: 'One-to-One' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

endersoncosta · 2022-07-25T12:51:07Z

@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.

To the initial setup this really doesn't has reason, but when we change the structure of an existing table, this can be necessary...
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.
Indeed, it is better in that simpler way.
Indeed.

pull main

dalssoft · 2022-07-27T23:40:33Z

the CLI created the default entity user.js:

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

when adding an entity with a reference:

constProject=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')})

endersoncosta · 2022-07-05T12:52:50Z

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

jhomarolo · 2022-07-05T13:00:31Z

+ <%if (props.ref) { 
+ props.ref.forEach(function(link){ 
+ if (link.relationship === 'One-to-One') { %>
+ // table.integer('<%-link.columnName %>').unsigned().nullable()


this comment is necessary?

the idea are to not create a migration that does not work, due to not has an order of migrations when we write the files. These relationships throw errors due the dependence dont exist yet ...

jhomarolo · 2022-07-05T13:02:11Z

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

endersoncosta · 2022-07-06T12:30:58Z

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 · 2022-07-07T12:20:57Z

Codecov Report

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

❗ 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%)`	⬇️
src/generators/utils.js	`83.33% <100.00%> (+1.85%)`	⬆️

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

dalssoft · 2022-07-08T01:50:27Z

@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.

dalssoft · 2022-07-08T03:04:24Z

 template: `infra/data/database/${db.toLowerCase()}/migration.ejs`,
 target: migrationFullPath,
- props: { table: `${camelCase(name)}s`, columns: columns }
+ props: { table: `${camelCase(name)}s`, externalColumnName: `${camelCase(name)}Id`, columns, ref, idColumn }


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:36Z

+ }
+
+ if (isArrayWithType(type) && herbs.entity.isEntity(type[0]))
+ refs.push({ table: `${camelCase(type[0].name)}s`, relationship: 'One-to-Many' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:52Z

+ if (herbs.entity.isEntity(type)) {
+ const typeSchema = type.prototype.meta.schema
+ const idRef = Object.values(typeSchema).find(column => column.options.isId)?.name
+ refs.push({ id: idRef, columnName: `${camelCase(name)}Id`, table: `${camelCase(type.name)}s`, relationship: 'One-to-One' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

endersoncosta · 2022-07-25T12:51:07Z

@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.

To the initial setup this really doesn't has reason, but when we change the structure of an existing table, this can be necessary...
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.
Indeed, it is better in that simpler way.
Indeed.

pull main

dalssoft · 2022-07-27T23:40:33Z

the CLI created the default entity user.js:

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

when adding an entity with a reference:

constProject=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')})

endersoncosta · 2022-07-05T12:52:50Z

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

jhomarolo · 2022-07-05T13:00:31Z

+ <%if (props.ref) { 
+ props.ref.forEach(function(link){ 
+ if (link.relationship === 'One-to-One') { %>
+ // table.integer('<%-link.columnName %>').unsigned().nullable()


this comment is necessary?

the idea are to not create a migration that does not work, due to not has an order of migrations when we write the files. These relationships throw errors due the dependence dont exist yet ...

jhomarolo · 2022-07-05T13:02:11Z

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

endersoncosta · 2022-07-06T12:30:58Z

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 · 2022-07-07T12:20:57Z

Codecov Report

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

❗ 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%)`	⬇️
src/generators/utils.js	`83.33% <100.00%> (+1.85%)`	⬆️

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

dalssoft · 2022-07-08T01:50:27Z

@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.

dalssoft · 2022-07-08T03:04:24Z

 template: `infra/data/database/${db.toLowerCase()}/migration.ejs`,
 target: migrationFullPath,
- props: { table: `${camelCase(name)}s`, columns: columns }
+ props: { table: `${camelCase(name)}s`, externalColumnName: `${camelCase(name)}Id`, columns, ref, idColumn }


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:36Z

+ }
+
+ if (isArrayWithType(type) && herbs.entity.isEntity(type[0]))
+ refs.push({ table: `${camelCase(type[0].name)}s`, relationship: 'One-to-Many' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

dalssoft · 2022-07-08T03:04:52Z

+ if (herbs.entity.isEntity(type)) {
+ const typeSchema = type.prototype.meta.schema
+ const idRef = Object.values(typeSchema).find(column => column.options.isId)?.name
+ refs.push({ id: idRef, columnName: `${camelCase(name)}Id`, table: `${camelCase(type.name)}s`, relationship: 'One-to-One' })


Both table and columns name should be snake_cases for DB to keep with the current standard.
ex: userId -> user_id

endersoncosta · 2022-07-25T12:51:07Z

@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.

To the initial setup this really doesn't has reason, but when we change the structure of an existing table, this can be necessary...
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.
Indeed, it is better in that simpler way.
Indeed.

pull main

dalssoft · 2022-07-27T23:40:33Z

the CLI created the default entity user.js:

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

when adding an entity with a reference:

constProject=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')})

feat: including relationships in migrations
f5be364

jhomarolo reviewed Jul 5, 2022
View reviewed changes

dalssoft mentioned this pull request Jul 5, 2022
herbs update - Entities with relationship #145
Open

test: fix the testcase
c6cc551

dalssoft requested changes Jul 8, 2022
View reviewed changes

endersoncostaand others added 7 commits July 25, 2022 09:58

feat: migration postgres execution improvement
4f10d6b

Merge pull request#1from herbsjs/main
3fcb965
pull main

Fix conflict
1d7d1b3

Merge commit '1d7d1b378105b81474057385c06f54ad7c7f6dc0'
335b92d

chore: fix dependencies
962252f

fix: fix herbarium require
a55258c

test: improving the test coverage
d8a12c6

dalssoft mentioned this pull request Sep 4, 2022
feat(update): automatically identify pk and fks into repository layer #150
Closed
4 tasks

feat: including relationships in migrations
f5be364

jhomarolo reviewed Jul 5, 2022
View reviewed changes

dalssoft mentioned this pull request Jul 5, 2022
herbs update - Entities with relationship #145
Open

test: fix the testcase
c6cc551

dalssoft requested changes Jul 8, 2022
View reviewed changes

endersoncostaand others added 7 commits July 25, 2022 09:58

feat: migration postgres execution improvement
4f10d6b

Merge pull request#1from herbsjs/main
3fcb965
pull main

Fix conflict
1d7d1b3

Merge commit '1d7d1b378105b81474057385c06f54ad7c7f6dc0'
335b92d

chore: fix dependencies
962252f

fix: fix herbarium require
a55258c

test: improving the test coverage
d8a12c6

dalssoft mentioned this pull request Sep 4, 2022
feat(update): automatically identify pk and fks into repository layer #150
Closed
4 tasks

feat: including relationships in migrations
f5be364

jhomarolo reviewed Jul 5, 2022
View reviewed changes

dalssoft mentioned this pull request Jul 5, 2022
herbs update - Entities with relationship #145
Open

test: fix the testcase
c6cc551

dalssoft requested changes Jul 8, 2022
View reviewed changes

endersoncostaand others added 7 commits July 25, 2022 09:58

feat: migration postgres execution improvement
4f10d6b

Merge pull request#1from herbsjs/main
3fcb965
pull main

Fix conflict
1d7d1b3

Merge commit '1d7d1b378105b81474057385c06f54ad7c7f6dc0'
335b92d

chore: fix dependencies
962252f

fix: fix herbarium require
a55258c

test: improving the test coverage
d8a12c6

dalssoft mentioned this pull request Sep 4, 2022
feat(update): automatically identify pk and fks into repository layer #150
Closed
4 tasks

feat: including relationships in migrations
f5be364

jhomarolo reviewed Jul 5, 2022
View reviewed changes

dalssoft mentioned this pull request Jul 5, 2022
herbs update - Entities with relationship #145
Open

test: fix the testcase
c6cc551

dalssoft requested changes Jul 8, 2022
View reviewed changes

endersoncostaand others added 7 commits July 25, 2022 09:58

feat: migration postgres execution improvement
4f10d6b

Merge pull request#1from herbsjs/main
3fcb965
pull main

Fix conflict
1d7d1b3

Merge commit '1d7d1b378105b81474057385c06f54ad7c7f6dc0'
335b92d

chore: fix dependencies
962252f

fix: fix herbarium require
a55258c

test: improving the test coverage
d8a12c6

dalssoft mentioned this pull request Sep 4, 2022
feat(update): automatically identify pk and fks into repository layer #150
Closed
4 tasks

feat: including relationships in migrations
f5be364

jhomarolo reviewed Jul 5, 2022
View reviewed changes

dalssoft mentioned this pull request Jul 5, 2022
herbs update - Entities with relationship #145
Open

test: fix the testcase
c6cc551

dalssoft requested changes Jul 8, 2022
View reviewed changes

endersoncostaand others added 7 commits July 25, 2022 09:58

feat: migration postgres execution improvement
4f10d6b

Merge pull request#1from herbsjs/main
3fcb965
pull main

Fix conflict
1d7d1b3

Merge commit '1d7d1b378105b81474057385c06f54ad7c7f6dc0'
335b92d

chore: fix dependencies
962252f

fix: fix herbarium require
a55258c

test: improving the test coverage
d8a12c6

dalssoft mentioned this pull request Sep 4, 2022
feat(update): automatically identify pk and fks into repository layer #150
Closed
4 tasks

feat: including relationships in migrations
f5be364

jhomarolo reviewed Jul 5, 2022
View reviewed changes

dalssoft mentioned this pull request Jul 5, 2022
herbs update - Entities with relationship #145
Open

test: fix the testcase
c6cc551

dalssoft requested changes Jul 8, 2022
View reviewed changes

endersoncostaand others added 7 commits July 25, 2022 09:58

feat: migration postgres execution improvement
4f10d6b

Merge pull request#1from herbsjs/main
3fcb965
pull main

Fix conflict
1d7d1b3

Merge commit '1d7d1b378105b81474057385c06f54ad7c7f6dc0'
335b92d

chore: fix dependencies
962252f

fix: fix herbarium require
a55258c

test: improving the test coverage
d8a12c6

dalssoft mentioned this pull request Sep 4, 2022
feat(update): automatically identify pk and fks into repository layer #150
Closed
4 tasks

feat: including relationships in migrations
f5be364

jhomarolo reviewed Jul 5, 2022
View reviewed changes

dalssoft mentioned this pull request Jul 5, 2022
herbs update - Entities with relationship #145
Open

test: fix the testcase
c6cc551

dalssoft requested changes Jul 8, 2022
View reviewed changes

endersoncostaand others added 7 commits July 25, 2022 09:58

feat: migration postgres execution improvement
4f10d6b

Merge pull request#1from herbsjs/main
3fcb965
pull main

Fix conflict
1d7d1b3

Merge commit '1d7d1b378105b81474057385c06f54ad7c7f6dc0'
335b92d

chore: fix dependencies
962252f

fix: fix herbarium require
a55258c

test: improving the test coverage
d8a12c6

dalssoft mentioned this pull request Sep 4, 2022
feat(update): automatically identify pk and fks into repository layer #150
Closed
4 tasks

feat: including relationships in migrations
f5be364

jhomarolo reviewed Jul 5, 2022
View reviewed changes

dalssoft mentioned this pull request Jul 5, 2022
herbs update - Entities with relationship #145
Open

test: fix the testcase
c6cc551

dalssoft requested changes Jul 8, 2022
View reviewed changes

endersoncostaand others added 7 commits July 25, 2022 09:58

feat: migration postgres execution improvement
4f10d6b

Merge pull request#1from herbsjs/main
3fcb965
pull main

Fix conflict
1d7d1b3

Merge commit '1d7d1b378105b81474057385c06f54ad7c7f6dc0'
335b92d

chore: fix dependencies
962252f

fix: fix herbarium require
a55258c

test: improving the test coverage
d8a12c6

dalssoft mentioned this pull request Sep 4, 2022
feat(update): automatically identify pk and fks into repository layer #150
Closed
4 tasks

Uh oh!

Conversation

endersoncosta commented Jul 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhomarolo commented Jul 5, 2022

Uh oh!

endersoncosta commented Jul 6, 2022

Uh oh!

codecovBot commented Jul 7, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dalssoft commented Jul 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

endersoncosta commented Jul 25, 2022

Uh oh!

dalssoft commented Jul 27, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

endersoncosta commented Jul 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhomarolo commented Jul 5, 2022

Uh oh!

endersoncosta commented Jul 6, 2022

Uh oh!

codecovBot commented Jul 7, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dalssoft commented Jul 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

endersoncosta commented Jul 25, 2022

Uh oh!

dalssoft commented Jul 27, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

endersoncosta commented Jul 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecovBot commented Jul 7, 2022•
edited
Loading

dalssoft commented Jul 27, 2022•
edited
Loading

codecovBot commented Jul 7, 2022•
edited
Loading

dalssoft commented Jul 27, 2022•
edited
Loading

codecovBot commented Jul 7, 2022•
edited
Loading

dalssoft commented Jul 27, 2022•
edited
Loading