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

[Fix] Class validator native isSemver does not handle v-prefix #10907

Merged
merged 3 commits into from
Mar 14, 2025

Conversation

prastoin
Copy link
Contributor

@prastoin prastoin commented Mar 14, 2025

Introduction

Under the hood class-validator isSemver uses https://github.com/validatorjs/validator.js/blob/master/src/lib/isSemVer.js which does not cover all semVer use cases

Even tho

Had a discussion with @charles was about to store in db ws version as vx.y.z. We felt like we wanted it to be stored as x.y.z, in my opinion APP_VERSION should reflect the tag used to be build the instance and not be updated
But we could extract only x.y.z from it at runtime

Also handling the v extraction in CD is IMO not the most reliable

Env var logging refactor

Now not stopping on first error log

Successfully compiled: 2128 files with swc (185.34ms)
Watching for file changes.
[Nest] 52686  - 03/14/2025, 6:28:33 PM   ERROR PG_DATABASE_URL should not be null or undefined
PG_DATABASE_URL must be a URL address
[Nest] 52686  - 03/14/2025, 6:28:33 PM   ERROR APP_VERSION must be a valid semantic version (e.g., 1.0.0)

/Users/paulrastoin/ws/twenty/packages/twenty-server/src/engine/core-modules/environment/environment-variables.ts:1019
    throw new Error("Environment variables validation failed")
          ^
Error: Environment variables validation failed
    at Object.validate (/Users/paulrastoin/ws/twenty/packages/twenty-server/src/engine/core-modules/environment/environment-variables.ts:1019:11)
    at Function.forRoot (/Users/paulrastoin/ws/twenty/node_modules/@nestjs/config/dist/config.module.js:67:45)
    at Object.<anonymous> (/Users/paulrastoin/ws/twenty/packages/twenty-server/src/engine/core-modules/environment/environment.module.ts:11:18)
    at Module._compile (node:internal/modules/cjs/loader:1256:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Function.Module._load (node:internal/modules/cjs/loader:960:12)
    at Module.require (node:internal/modules/cjs/loader:1143:19)
    at require (node:internal/modules/cjs/helpers:121:18)
    at Object.<anonymous> (/Users/paulrastoin/ws/twenty/packages/twenty-server/dist/src/database/typeorm/typeorm.module.js:14:28)

@prastoin prastoin marked this pull request as ready for review March 14, 2025 17:49
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR improves semantic version validation by replacing the built-in validator with a custom implementation that better handles version strings and enhances environment variable error reporting.

  • Added IsTwentySemVerValidator in /packages/twenty-server/src/engine/core-modules/environment/decorators/is-twenty-semver.decorator.ts using semver package for more robust version validation
  • Modified environment variable validation in environment-variables.ts to show all validation errors instead of stopping at first error
  • Improved error messages to include examples (e.g., "must be a valid semantic version (e.g., 1.0.0)")
  • Removed dependency on validatorjs in favor of more reliable semver package

2 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

@prastoin prastoin enabled auto-merge (squash) March 14, 2025 17:53
@prastoin prastoin merged commit d0e80a5 into main Mar 14, 2025
32 checks passed
@prastoin prastoin deleted the fix-twenty-semver-decorator branch March 14, 2025 18:00
charlesBochet pushed a commit that referenced this pull request Mar 14, 2025
# Introduction
We want the APP_VERSION to be able to contains pre-release options, in a
nutshell to be semVer compatible.
But we want to have workspace, at least for the moment, that only store
`x.y.z` and not `vx.y.z` or `x.y.z-alpha` version in database

Explaining this refactor

Related #10907
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.

2 participants