search

edgedb / edgedb-js

The official TypeScript/JS client library and query builder for EdgeDB
https://edgedb.com
Apache License 2.0
514 stars 65 forks source link

resolveConnectionParams listing incorrect source for database/branch #1069

Closed mfenn closed 3 months ago

mfenn commented 3 months ago

Describe the bug resolveConnectionParams seems to erroneously report the source of a branch as "default" when the EDGEDB_BRANCH env variable is used, though the branch specified by EDGEDB_BRANCH is correctly used.

Reproduction We use resolveConnectionParams to log connection parameters when our application starts up. I ran with the EDGEDB_INSTANCE and EDGEDB_BRANCH=example (but not the default main).

Parameter          Value                                    Source
---------          -----                                    ------
host               <host>                                    'EDGEDB_INSTANCE' environment variable
port               5656                                      'EDGEDB_INSTANCE' environment variable
database           example (default)                         default
...

Expected behavior I would expect that it would report the source of the branch as the 'EDGEDB_BRANCH' environment variable.

Versions (please complete the following information):

scotttrinh commented 3 months ago

Does the branch parameter show the correct branch here?

mfenn commented 3 months ago

I don't actually see a branch parameter, just host, port, database, user, password, tlsCAData, tlsSecurity, and waitUntilAvailable.

scotttrinh commented 3 months ago

Hmmm, it should be there via: https://github.com/edgedb/edgedb-js/blob/9c60b14e2b0ab55f2b81e36027a2ba0707a7142f/packages/driver/src/conUtils.ts#L358-L360

Oh! Maybe you're using explainConfig() https://github.com/edgedb/edgedb-js/blob/9c60b14e2b0ab55f2b81e36027a2ba0707a7142f/packages/driver/src/conUtils.ts#L390

It is indeed missing from that table. Will fix.

mfenn commented 3 months ago

Oh yeah, sorry, should have been more clear.

scotttrinh commented 3 months ago

Oh yeah, sorry, should have been more clear.

No problem! Should be fixed up in just a few minutes.

scotttrinh commented 3 months ago

@mfenn

Can you try the latest canary (https://www.npmjs.com/package/edgedb/v/1.6.0-canary.20240730T180738) and let me know if it works for you? If so, I'll publish a patch release.

mfenn commented 3 months ago

Yep, looks good. Is there any difference between database and branch at this point?

scotttrinh commented 3 months ago

@mfenn

Is there any difference between database and branch at this point?

"Branch" is a superset of "database". It has a few more features (like being able to copy data when creating a new branch, and some CLI commands) but from a purely data/connection perspective, branch supersedes database as a concept.