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

Remove database configuration option, hardcode 'fxa' #290

Closed
nthuemmel opened this Issue Jan 2, 2018 · 12 comments

Comments

Projects
None yet
3 participants
@nthuemmel
Copy link

nthuemmel commented Jan 2, 2018

patch-061-062.sql contains the hardcoded database name 'fxa' in the following statement: ALTER DATABASE fxa CHARACTER SET utf8mb4 COLLATE utf8mb4_bin;

This bricks all configurations which contain a different master/slave database name. When using such a configuration, node bin/db_patcher.js fails with an error similar to the following:

{"Timestamp":1514917609831000000,"Logger":"fxa-auth-db-server","Type":"bin.db_patcher.patch-error","Severity":2,"Pid":3929,"EnvVersion":"2.0","Fields":{"err":"Error: ER_DBACCESS_DENIED_ERROR: Access denied for user 'fxacc'@'localhost' to database 'fxa'"}}

@vladikoff

This comment has been minimized.

Copy link
Member

vladikoff commented Jan 4, 2018

from meeting:

One plan is to remove the config option and force fxa db name:

      database: {
        doc: 'The database to connect to for MySql',
        default: 'fxa',
        env: 'MYSQL_DATABASE',
},

The fix on our side would be to use templates for migrations, we are not planning to do that at this time.

@nthuemmel

This comment has been minimized.

Copy link

nthuemmel commented Jan 4, 2018

Removing the configuration option seems like a reasonable solution. Just mention that the database name fxa is required in the README, so other users don't run into problems. Thanks for the response!

@vladikoff vladikoff changed the title Hardcoded database name in patch-061-062.sql Remove database configuration option, hardcode 'fxa' Jan 4, 2018

@deeptibaghel

This comment has been minimized.

Copy link
Contributor

deeptibaghel commented Mar 6, 2018

@vladikoff , i am done with the development and would like to work on this issue for my outreachy contribution.

@vladikoff

This comment has been minimized.

Copy link
Member

vladikoff commented Mar 6, 2018

@deeptibaghel Sounds good! Please send a pull request to this issue with the changes.

As part of the patch you need to move the database names from configuration options into a constants file.

Here it is in 2 places:

and

Create a new file called constants.js in fxa-auth-db-mysql/lib and export the database name. (example https://stackoverflow.com/a/21247500 )

Later import that constant name whereever the database name was used before via the configuration file. (example

database: config.General.db_name
},
slave: {
host: config.General.db_dnsname,
user: config.General.db_username,
password: config.General.db_password,
database: config.General.db_name
there could be other places where we used the value from the configuration)

Lastly create a simple test file for constants.js to assert that your new file exports the database name properly.

Let me know if you have questions.

@deeptibaghel

This comment has been minimized.

Copy link
Contributor

deeptibaghel commented Mar 6, 2018

Thanks @vladikoff , i am working as suggested by you, how do we handle the test for metrics after the change. Right now it is looking db_name = qux

  1. DB metrics run, with mocked queries:

    AssertionError [ERR_ASSERTION]: mysql.connect master.database option was correct from assert.equal(options.master.database, 'qux', 'mysql.connect master.database option was correct')

    • expected - actual

    -fxa
    +qux

@vladikoff

This comment has been minimized.

Copy link
Member

vladikoff commented Mar 6, 2018

@deeptibaghel you need to modify the assertion, fxa should be value now because that is a constant. You can pull the value from constants.js in the test file as well.

@deeptibaghel

This comment has been minimized.

Copy link
Contributor

deeptibaghel commented Mar 7, 2018

@vladikoff , i need some more clarification on this, do we need to remove entire database block from
config.js i.e.
database: {
doc: 'The database to connect to for MySql',
default: 'fxa',
env: 'MYSQL_DATABASE',
},
Then is there a way to assign it in the same file from constants after loading environment specific json file?

or we keep it and set the value from constants, then we still leave the option to configure it from environment specific json file ?

@vladikoff

This comment has been minimized.

Copy link
Member

vladikoff commented Mar 7, 2018

do we need to remove entire database block from

Yeap that whole block, also there are 2 of them, search for MYSQL_SLAVE_DATABASE

is there a way to assign it in the same file from constants after loading environment specific json file?

Per issue title we want to remove configuration for database names (title: Remove database configuration option), so constants.js will just have const DATABASE_NAME = 'fxa' or something. There will be no way to change that or configure that

@deeptibaghel

This comment has been minimized.

Copy link
Contributor

deeptibaghel commented Mar 7, 2018

Then how do we pass it back to conf object from constants file because every where else the entire conf object is being used.

@vladikoff

This comment has been minimized.

Copy link
Member

vladikoff commented Mar 7, 2018

Then how do we pass it back to conf object

The database name won't be part of the conf object anymore. You would require the name from the constants file. Is there a specific code path that you are worried about?

@deeptibaghel

This comment has been minimized.

Copy link
Contributor

deeptibaghel commented Mar 7, 2018

so after i remove it, i get error "No database selected" in test-mysql, which file should i set it

@vladikoff

This comment has been minimized.

Copy link
Member

vladikoff commented Mar 7, 2018

@deeptibaghel you need to add your new database name constant into the db_patcher as well.

See https://github.com/mozilla/fxa-auth-db-mysql/blob/master/bin/db_patcher.js#L14
So something like this:

var options = Object.assign({}, config.master)
options.database = Constants.DATABASE_NAME // <-- New thing

// v--- other stuff below that is already there.
options.dir = path.join(__dirname, '..', 'lib', 'db', 'schema')
options.patchKey = config.patchKey
options.metaTable = 'dbMetadata'
options.patchLevel = patch.level

Might need some tweaking. Make sure to search and find all the places where we used config.master and config.slave options and add the name constant in there

deeptibaghel added a commit to deeptibaghel/fxa-auth-db-mysql that referenced this issue Mar 7, 2018

deeptibaghel added a commit to deeptibaghel/fxa-auth-db-mysql that referenced this issue Mar 7, 2018

deeptibaghel added a commit to deeptibaghel/fxa-auth-db-mysql that referenced this issue Mar 9, 2018

@vladikoff vladikoff closed this in c2e21dd Mar 9, 2018

@wafflebot wafflebot bot removed the waffle:backlog label Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment