Fix the unreachable unknown-database-type guard (#1091) - #1092
Open
gangster wants to merge 1 commit into
Open
Conversation
'!name in config' parses as '(!name) in config', which tests whether the boolean false is a key of the config object. It never is, so the throw was dead code for every input. An unrecognised --db value instead failed two lines later with "TypeError: config[name] is not a function", pointing at the wrong line. Use Object.hasOwn rather than the minimal '!(name in config)': plain 'in' walks the prototype chain, so --db=toString would have passed the guard and handed config.toString(parts), the string "[object Object]", on as the connection settings. Unknown types now report "unknown database type '<name>'" as intended. sqlite, mysql, sqlite:<path> and the no-argument default are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1091.
!name in configparses as(!name) in config, so the guard tested whether the booleanfalsewas a key of the config object. It never is, so the throw was unreachable for every input, and an unrecognised--dbvalue failed two lines later instead:Why
Object.hasOwnrather than!(name in config)The minimal correction fixes the precedence but leaves a second hole, because
inwalks the prototype chain andconfigis a plain object literal. On currentmain:config.toString(parts)returns the string"[object Object]", which is passed on as the connection settings and fails somewhere less obvious indb.js.!(name in config)would not catch that;hasOwndoes, at the same length. Same bug class as #1089.Verification
Unknown types now produce the intended message:
Valid types are unchanged:
--db=sqlitetype=sqlite connect={"name":":memory:"}mysqltype=mysql connect={"host":"db","port":3306,...}sqlite:../bin/events.dbtype=sqlite connect={"name":".../bin/events.db"}type=mysqlThe
sqlite:<path>row is the one worth noting: the guard sits immediately afterdbType.split(':'), so it confirms the parts handling still works.npm testis 53/53, andnpm test --db=sqlitealso passes.No test
config.jsreads the environment at require time and exports a plain object, so there is no seam to assert against without exportinggetDatabaseConfigfor testing. That is the house pattern elsewhere (calEventValidator,ical.js), but it widens the module's surface for a one-line fix, so I left it alone and verified manually as above. Happy to add it if you would rather have the coverage.Object.hasOwnneeds Node 16.9+;docker-compose.ymlpinsnode:24.15.0-slim.