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

feat(update): automatically identify pk and fks into repository layer #150

Closed
wants to merge 4 commits into from
Closed

Conversation

danielsous
Copy link
Contributor

Fixes #26

Proposed Changes

  1. Use the power of the CLI to identify primary keys and foreign keys directly from the entity's implementation.

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
  • Remember to check if code coverage decrease, if so, please implement the necessary tests

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #150 (e0b4396) into main (56e99b7) will increase coverage by 1.12%.
The diff coverage is 100.00%.

❗ Current head e0b4396 differs from pull request most recent head d9e29bc. Consider uploading reports for the commit d9e29bc to get more accurate results

@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
+ Coverage   86.28%   87.41%   +1.12%     
==========================================
  Files          25       25              
  Lines         423      437      +14     
==========================================
+ Hits          365      382      +17     
+ Misses         58       55       -3     
Impacted Files Coverage Δ
...rs/src/infra/database/repositories/repositories.js 97.50% <100.00%> (+1.34%) ⬆️
...rators/src/infra/database/migrations/migrations.js 94.73% <0.00%> (+2.63%) ⬆️
src/commands/herbs.js 100.00% <0.00%> (+50.00%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@jhomarolo jhomarolo requested a review from dalssoft July 27, 2022 14:27
@dalssoft
Copy link
Member

@danielsous the current implementation is not handling multiples IDs:

const Project =
    entity('Project', {
        id1: id(String),
        id2: id(String),
        name: field(String),
    })

@danielsous
Copy link
Contributor Author

@dalssoft, @italojs, @jhomarolo

@@ -8,7 +8,9 @@ class <%- props.name.pascalCase %>Repository extends Repository {
super({
entity: <%- props.name.pascalCase %>,
table: "<%- props.table %>",
knex: connection
ids: [<% for (const [index, value] of Object.entries(props.primaryKeyFields)) { %><%if(index != 0) { %>, <% } %>'<%= value %>'<% } %>],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the code seems duplicated in other files.

I think this should be refactored

fs.rmSync(path.resolve(process.cwd(), `${projectName}`), { recursive: true })
})

it('Should I create a new repository with foreign key', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be a test without the foreing key

@dalssoft
Copy link
Member

dalssoft commented Sep 4, 2022

@danielsous great contrib! having support for entities relationship is one of the most important feature for now.

just a few points:

@dalssoft
Copy link
Member

dalssoft commented Sep 4, 2022

I don't think it is necessary to explicitly inform PKs (ids: [) on the repository since it is able to detect ids using metadata.

@dalssoft
Copy link
Member

dalssoft commented Sep 4, 2022

1 to 1 scenario:

Testing with two entities with relationship between then:

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

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

The repository (ProjectRepository) was created with foreignKeys: [{ user_id: String ,}] which is correct, but the migration (20220904220920_projects.js) had table.undefined('user') as the field definition, which is a bug.

@dalssoft
Copy link
Member

dalssoft commented Sep 4, 2022

1 to Many scenario:

Testing with two entities with relationship between then:

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

const Project =
        entity('Project', {
          id: id(String),
          user: field([User]),     // <----- Many users
        })

When herbs update it throws a expection:

Generating Entities
Generating Repositories
  New: C:\Users\David\Projects\herbs-project\src\infra\data\repositories\userRepository.js
C:\Users\David\Projects\herbs-cli\node_modules\gluegun\build\index.js:15
    throw up;
    ^

TypeError: Cannot read properties of undefined (reading 'meta')
    at C:\Users\David\Projects\herbs-cli\src\generators\src\infra\database\repositories\repositories.js:13:51
    at Array.map (<anonymous>)
    at generateForeignKeysField (C:\Users\David\Projects\herbs-cli\src\generators\src\infra\database\repositories\repositories.js:10:25)
    at generateRepositories (C:\Users\David\Projects\herbs-cli\src\generators\src\infra\database\repositories\repositories.js:40:45)

@dalssoft
Copy link
Member

dalssoft commented Sep 4, 2022

@danielsous I think your PR is very close to the effort started by this PR #144 created by @endersoncosta regarding issue #145 . You guys should join forces to finish it. As I said, this feature is veeery import for the project

@danielsous danielsous closed this by deleting the head repository Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generating relations into repository layer
4 participants