Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

fix(server): permit null values in devices response #1124

Merged
merged 1 commit into from Nov 22, 2015
Merged

Conversation

philbooth
Copy link
Contributor

Fixes #1123 but depends on changes in mozilla/fxa-auth-db-mysql#117 so do not merge until after that PR has been accepted.

This issue was caused by my misunderstanding of Joi's optional(), which doesn't permit null values. It was exacerbated by yet another gaping hole in my test coverage, this time in fxa-auth-db-mysql, which meant the memory backend and the MySQL backend were behaving differently with regards to the initialisation of missing fields in the devices table.

And then I stupidly to forgot to test device registration against the MySQL backend, so the aforementioned discrepancy masked this bug until @jrgm found it when he deployed train 50. Apologies @jrgm (and everybody else).

I've updated npm-shrinkwrap.json here for the purposes of making travis pass, but it will need to be updated properly when the db change is in master.

@philbooth
Copy link
Contributor Author

Just to add, in case it isn't clear: the db change doesn't touch any production code. I don't know where this leaves things in terms of trains but, if it is decided to patch train 50 with this fix, there's no need to also patch the db.

@rfk
Copy link
Contributor

rfk commented Nov 22, 2015

And then I forgot to test device registration against the MySQL backend

We shouldn't have to explicitly remember these things. Is there a problem with our travis config on this repo?

@rfk
Copy link
Contributor

rfk commented Nov 22, 2015

I filed #1125 to remind us to take a look at why travis didn't catch this; code here looks good, r+

rfk added a commit that referenced this pull request Nov 22, 2015
fix(server): permit null values in devices response
@rfk rfk merged commit 7718e0c into master Nov 22, 2015
@rfk
Copy link
Contributor

rfk commented Nov 22, 2015

@philbooth I wonder if all this fragility is the reason we had disabled response schema validation on some routes in the past...

@philbooth
Copy link
Contributor Author

I wonder if all this fragility is the reason we had disabled response schema validation on some routes in the past.

Yeah, I can believe that. Fwiw, I still like the design-by-contractness of it and it was only because of some missing test coverage that the fragility was able to be an issue.

@philbooth philbooth deleted the phil/issue-1123 branch November 23, 2015 08:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants