feat: add some very basic validation to Metric struct

[darahayes]

Description

This PR adds a function Validate() (valid bool, reason string) to the Metric struct and uses that function in the metrics handler to check for some basic validation cases. clientId not existing and Data not existing or being empty object.

This prevents us from saving empty entries in postgres. Unit tests have been added for the Validate() function.

Verification steps

I've constructed some curl requests and expected responses so you can try out. For now the server returns the same data that is saved in the DB. This is handy in development.

Case 1

The happy case - full payload. (Excluding client timestamp - will look after this in a separate PR)

curl -X POST \
  http://localhost:3000/metrics \
  -H 'Cache-Control: no-cache' \
  -H 'Content-Type: application/json' \
  -H 'Postman-Token: 77100fc4-f06a-7098-69e4-dfdcaa207e98' \
  -d '{
  "clientId": "12345",
  "data": {
  	"app": {
    "id": "com.example.someApp",
    "sdkVersion": "2.4.6",
    "appVersion": "256"
	},
	"device": {
    	"platform": "android",
    	"platformVersion": "27"
	}
  }
}'

result:

{
    "clientId": "12345",
    "data": {
        "app": {
            "id": "com.example.someApp",
            "sdkVersion": "2.4.6",
            "appVersion": "256"
        },
        "device": {
            "platform": "android",
            "platformVersion": "27"
        }
    }
}

Case 2

app data is included but not device metrics. i.e. only a subset of all possible fields, but still valid.

curl -X POST \
  http://localhost:3000/metrics \
  -H 'Cache-Control: no-cache' \
  -H 'Content-Type: application/json' \
  -H 'Postman-Token: 2ba02c83-8ba8-7b3b-2ddf-c63de048cb8d' \
  -d '{
  "clientId": "12345",
  "data": {
  	"app": {
    "id": "com.example.someApp",
    "sdkVersion": "2.4.6",
    "appVersion": "256"
	}
  }
}'

result:

{
    "clientId": "12345",
    "data": {
        "app": {
            "id": "com.example.someApp",
            "sdkVersion": "2.4.6",
            "appVersion": "256"
        }
    }
}

Case 3

Client sends an empty object:

curl -X POST \
  http://localhost:3000/metrics \
  -H 'Cache-Control: no-cache' \
  -H 'Content-Type: application/json' \
  -H 'Postman-Token: 2fd4aa64-43ad-a168-3c57-3c31b250b0ba' \
  -d '{
  
}'

Result:

{
    "error": "Bad Request",
    "message": "missing clientId in payload",
    "statusCode": 400
}

Case 4

Data field is not included

curl -X POST \
  http://localhost:3000/metrics \
  -H 'Cache-Control: no-cache' \
  -H 'Content-Type: application/json' \
  -H 'Postman-Token: 3d3c1945-d191-55e4-0b52-eeffbdb64afc' \
  -d '{
  "clientId": "12345"
}'

result:

{
    "error": "Bad Request",
    "message": "missing metrics data in payload",
    "statusCode": 400
}

Case 5

Data field is there but empty object

curl -X POST \
  http://localhost:3000/metrics \
  -H 'Cache-Control: no-cache' \
  -H 'Content-Type: application/json' \
  -H 'Postman-Token: 3d3c1945-d191-55e4-0b52-eeffbdb64afc' \
  -d '{
  "clientId": "12345",
  "data": {
  	
  }
}'

result:

{
    "error": "Bad Request",
    "message": "missing metrics data in payload",
    "statusCode": 400
}

[pb82]

will this be an empty string if not submitted or nil?

[paolobueno]

Looks like it can only be nil if it's a pointer: https://stackoverflow.com/questions/12703243/what-is-the-zero-for-string

Learning this now myself too :D

[pb82]

Thanks, good to know! I have not much of an Idea about Golang ;)

[darahayes]

As part of decoding JSON into a struct, if a field is omitted it is instantiated with it's 'zero' value. The zero value for a string is "".

You see later on that we check if m.Data is nil. That's because the Data field expects a type *MetricData which is a pointer as opposed to the value itself. The zero value of a pointer is nil, hence the nil check for metric.Data but empty string check for ClientId. Does that make sense @pb82 ?

[pb82]

@darahayes yes makes sense, thanks for explaining that

[pb82]

shouldn't the message be empty metric is invalid?

[darahayes]

The validate function executes checks in sequence where the first check is the clientId. For that reason this was the behaviour I intended. Happy to change it it but I think it's good

[pb82]

I was just going by the name of the test which mentions Empty metrics. I guess missing clientId is still correct in that case.

[pb82]

LGTM, just a small comment

[paolobueno]

Verified locally

[paolobueno]

@darahayes do you think it would be worthwhile converting your curl statements into integration tests?
Running http + httptest.Server.

[darahayes]

Definitely. I'm going to create a ticket for e2e style tests that will behave in this way.