-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update required fields and validation error message #1
base: master
Are you sure you want to change the base?
Changes from all commits
028a46c
2b2a55f
aab20c9
3a8c6cb
8cc3fc0
dd08bda
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,32 +2,59 @@ import * as functions from "firebase-functions"; | |
import { store } from "./database"; | ||
|
||
export const save = functions.https.onRequest((request, response) => { | ||
const event = { | ||
// Client-provided values | ||
uid: request.body.uid, | ||
client: request.body.client, | ||
key: request.body.key, | ||
value: request.body.value, | ||
const body = request.body; | ||
console.log(`Body = ${JSON.stringify(body)}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is useful for debugging, but I'd remove this for production, since it will result in a lot of logs with unclear retention policies. |
||
|
||
// Optional (TODO: make it required like the other fields) | ||
sent: request.body.sent, | ||
const event = Object.assign({}, body, { | ||
receivedAt: new Date().toISOString() | ||
}); | ||
|
||
// Additional values | ||
received: new Date().toISOString() | ||
}; | ||
const requiredFields: string[] = [ | ||
// A key used to identify the value in this event (it need not be unique). | ||
// For example, this could be the type of the event. | ||
'key', | ||
|
||
// The data that the client wants to store in the database. | ||
'value', | ||
|
||
// An identifier for the user associated with this event. | ||
'uid', | ||
|
||
// An identifier for the client issuing this request (e.g., the app | ||
// version). Think of this as the client user-agent. | ||
'clientVersion', | ||
|
||
// Timestamp for when the client generated the event. NOTE: This should be | ||
// when the event actually occurred on the client and NOT when it was sent | ||
// to the server. | ||
'timestamp', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might be nice for the two timestamps to have more "symmetrical" names, e.g., (In general, I want to nudge us to use the server timestamp; the client timestamp should be used primarily for debugging, because it can be way off due to clock skew.) |
||
|
||
// The timestamp for when the server received this event from the client. | ||
'receivedAt' | ||
]; | ||
|
||
// Validate data | ||
if (!event.uid || !event.client || !event.key || !event.value) { | ||
response.status(400).send("Bad Request"); | ||
return; | ||
const missingFields = []; | ||
for (const fieldName of requiredFields) { | ||
// Unsafe casting to get around type error: | ||
// https://stackoverflow.com/a/57192972. | ||
if (event[fieldName] === undefined) { | ||
missingFields.push(fieldName); | ||
} | ||
} | ||
|
||
if (missingFields.length > 0) { | ||
const errorMsg = `Missing required fields: [${missingFields.join(', ')}]`; | ||
response.status(400).send(errorMsg); | ||
} | ||
|
||
// Store data | ||
store(event) | ||
.then(() => { | ||
response.send("OK"); | ||
response.sendStatus(200); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure about the default behavior of the 200 response on Firebase, but can you make sure we still get some sort of text acknowledgment at the end of request? It's useful for manual debugging. |
||
}) | ||
.catch(() => { | ||
response.status(500).send("Internal Server Error"); | ||
.catch((e) => { | ||
response.sendStatus(500); | ||
console.log(`Error saving to the DB. Request: ${JSON.stringify(body)}. Error: ${e}.`); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer custom fields to be down in
value
. That way, the top level has a finite and final list of fields, and we know that any additional or metadata fields we might add are out of the client's control.