Skip to content

FIX #2719: enforce name argument of migration generate command#6690

Merged
pleerock merged 2 commits intotypeorm:masterfrom
akosasante:bugfix-2719-migrate-command
Sep 26, 2020
Merged

FIX #2719: enforce name argument of migration generate command#6690
pleerock merged 2 commits intotypeorm:masterfrom
akosasante:bugfix-2719-migrate-command

Conversation

@akosasante
Copy link
Contributor

@akosasante akosasante commented Sep 9, 2020 •
edited
Loading

User error sometimes leads to running the migration generate command with invalid parameters. If the -n argument for the name of the migration class is completely missing, it returns a useful error to the user:

> typeorm migration:generate -f ormconfig -c development
> Generates a new migration file with sql needs to be executed to update schema.

Options:
  -h, --help        Show help                                          [boolean]
  -c, --connection  Name of the connection on which run a query.
                                                            [default: "default"]
  -n, --name        Name of the migration class.                      [required]
  -d, --dir         Directory where migration should be created.
  -f, --config      Name of the file with connection configuration.
                                                          [default: "ormconfig"]
  -v, --version     Show version number                                [boolean]

Missing required argument: n

However, if the user includes the -n argument but forgets to pass in a value for that option, currently the yargs library is interpreting that as a positional argument, with a value of true (https://github.com/yargs/yargs/blob/master/docs/advanced.md#positional-arguments). This then results in the aforementioned TypeError: str.replace is not a function error when name is passed into the camelCase method.

{
  _: [ 'migration:generate' ],
  f: 'ormconfig',
  config: 'ormconfig',
  c: 'development',
  connection: 'development',
  n: true,
  name: true,
  '$0': 'node_modules/.bin/typeorm'
}
boolean
Error during migration generation:
TypeError: str.replace is not a function
    at Object.camelCase (/Users/aasante/h-dev/TradeMachine/trade-machine-server/node_modules/typeorm/util/StringUtils.js:12:16)
    at Function.MigrationGenerateCommand.getTemplate (/Users/aasante/h-dev/TradeMachine/trade-machine-server/node_modules/typeorm/commands/MigrationGenerateCommand.js:168:48)
    at Object.<anonymous> (/Users/aasante/h-dev/TradeMachine/trade-machine-server/node_modules/typeorm/commands/MigrationGenerateCommand.js:117:64)
    at step (/Users/aasante/h-dev/TradeMachine/trade-machine-server/node_modules/tslib/tslib.js:136:27)
    at Object.next (/Users/aasante/h-dev/TradeMachine/trade-machine-server/node_modules/tslib/tslib.js:117:57)
    at fulfilled (/Users/aasante/h-dev/TradeMachine/trade-machine-server/node_modules/tslib/tslib.js:107:62)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)

The goal of this PR is to improve the messaging returned to the user in this case; hopefully making it more obvious how to resolve.

Previous work was done in this area to return a message if the name argument is empty: e68538a
but it doesn't work here because of the argument being coerced into a boolean value.

This PR adds the type: "string" to the name argument options passed to yarg. This tells the yarg library to treat this argument as a string at all times, and a missing value will be interpreted as an empty string, rather than a positional argument. With this PR we get the following:

{
  _: [ 'migration:generate' ],
  f: 'ormconfig',
  config: 'ormconfig',
  c: 'development',
  connection: 'development',
  n: '',
  name: '',
  '$0': 'node_modules/.bin/typeorm'
}
Please specify a migration name using the `-n` argument

Note to maintainers: I am happy to add tests if wanted; but I wasn't able to find any existing tests for what's returned by the various CLI commands; and this was a fairly small change.

Closes (or at least, resolves the confusion brought up by): #2719, #4798, #4805

Akosua Asante added 2 commits September 9, 2020 16:56
fix: enforce name argument of migration generate command
28745ac 
enforce the type of the name argument in order to return a more useful error message when the user forgets to provide a migration name to the generate migration command

Closes: typeorm#2719, typeorm#4798, typeorm#4805
fix: update error message text for generate migration command when mi…
d691893 
…ssing name argument

enforce the type of the name argument in order to return a more useful error message when the user forgets to provide a migration name to the generate migration command

Closes: typeorm#2719, typeorm#4798, typeorm#4805
@pleerock
Copy link
Member

pleerock commented Sep 26, 2020

Looks good, thank you for your contribution!

@pleerock pleerock merged commit dfcb2db into typeorm:master Sep 26, 2020
zaro pushed a commit to zaro/typeorm that referenced this pull request Jan 12, 2021
@akosasante
fix: enforce name argument of migration generate command (typeorm#2719)…
526a21a 
… (typeorm#6690)

* fix: enforce name argument of migration generate command

enforce the type of the name argument in order to return a more useful error message when the user forgets to provide a migration name to the generate migration command

Closes: typeorm#2719, typeorm#4798, typeorm#4805

* fix: update error message text for generate migration command when missing name argument

enforce the type of the name argument in order to return a more useful error message when the user forgets to provide a migration name to the generate migration command

Closes: typeorm#2719, typeorm#4798, typeorm#4805

Co-authored-by: Akosua Asante <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nitaking nitaking nitaking approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@akosasante @pleerock @nitaking