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: generate id field using uuid #191

Merged

Conversation

rodrigodosanjosoliveira
Copy link
Contributor

@rodrigodosanjosoliveira rodrigodosanjosoliveira commented Jul 4, 2023

Fixes #158

Proposed Changes

  1. Replace uuid package with the native Node.js crypto module:
  • Removed the dependency on uuid package from package.json.
  • Updated all UUID generation to use the native randomUUID() function from the Node.js crypto module.
  1. Update Migration Generation for UUID Handling:
  • Improved migration generation logic to handle UUID columns appropriately based on the database.
  • For PostgreSQL, the id field is generated using uuid_generate_v4() as the default value for primary keys.

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 decreases, 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 4, 2023

Codecov Report

Merging #191 (d137937) into main (1099e97) will not change coverage.
The diff coverage is n/a.

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

@@           Coverage Diff           @@
##             main     #191   +/-   ##
=======================================
  Coverage   68.75%   68.75%           
=======================================
  Files          28       28           
  Lines         560      560           
=======================================
  Hits          385      385           
  Misses        175      175           
Impacted Files Coverage Δ
src/generators/src/packagejson.js 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dalssoft
Copy link
Member

dalssoft commented Jul 4, 2023

Thanks @rodrigodosanjosoliveira for the contribution

@dalssoft
Copy link
Member

dalssoft commented Jul 4, 2023

On the migrations you would need something like this:
table.uuid('id').primary().defaultTo(knex.raw('uuid_generate_v4()')) (Postgress)
where this line could change for each DB supported (PG, SQL Server, MySQL, etc)

@rodrigodosanjosoliveira
Copy link
Contributor Author

rodrigodosanjosoliveira commented Oct 30, 2024

@dalssoft
I updated the migration generation logic to handle UUID columns according to the database. For Postgres, it now uses uuid_generate_v4() as the default value for UUID primary keys. Thanks for pointing this out!

@dalssoft dalssoft merged commit b460b3f into herbsjs:main Oct 30, 2024
1 check passed
@rodrigodosanjosoliveira rodrigodosanjosoliveira deleted the feature/generate-id-field branch October 30, 2024 17:19
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.

Uses ObjectID on id field for all databases choices
2 participants