Should this repo keep all of it's bufferization at the API boundary?
Closed this issue · 8 comments
In #315, some changes were made to hex-encode the email param on a bunch of endpoints. From a functional standpoint I have no problem with them, it's all good stuff.
But from a code-cleanliness standpoint it seems like the bufferization got moved to the wrong place.
Two reasons:
-
For other params, we do the bufferization as close to the API boundary as possible, in
db-server/index.js. Semantically, that feels like the right place for argument-marshalling to happen, and these changes break with the convention. -
By doing the bufferization closer to the db layer, we have to implement it twice on each endpoint. Once in the memory backend and again in the MySQL backend. Better to only do it once at the API boundary, no?
Should we move the bufferization so it happens in db-server/index.js again?
@philbooth yeah we should make it consistent. There was some bufferization magic added in https://github.com/mozilla/fxa-auth-db-mysql/blob/master/db-server/index.js#L57-L76 that might help with that
I believe all endpoints that use an email as a parameter, should be getting them as a hex encoded value, so we are a little bit more consistent there.
Should we move the bufferization so it happens in db-server/index.js again?
Do you mean so that the database methods get the un-hex email? That change makes sense to me since it is a little clunky to remember that you have to pass a hexed email.
AKA all db methods should get email AS utf8 from index.js
@vladikoff can I work on this ?
@deeptibaghel sure! let us know if you have questions
@vladikoff Do we need to utf8 encode all email parameters in index.js i.e Buffer.from(email,'utf8') ?
[10:51:59] <vladikoff> deepti, we already have this function that converts hex to buffer: https://github.com/mozilla/fxa-auth-db-mysql/blob/master/db-server/index.js#L57
[10:52:23] <vladikoff> deepti, you can introduce a new function in that module and use
[10:52:49] <vladikoff> deepti, use it to to do `.from(email,'hex').toString('utf8')` on the fly
[11:58:31] <pb> deepti, vladikoff: sorry, late answer to the question, but it depends what you mean by "all email parameters in index.js". we only want to do it for the ones that are being bufferized in the db layer, we don't want to do it for email params on other endpoints.
[11:58:45] <pb> (maybe that was already obvious, clarifying just in case it wasn't)
[11:58:55] <vladikoff> pb++