Handles inconsistent JSON-encoding on query property. #9
Conversation
gohelper.go
Outdated
| mappedRawQuery, ok := rels[k][0].RawQuery.(map[string]interface{}) | ||
| if !ok { | ||
| // Handle empty array case | ||
| rels[k][0].Query.IsMaster = false |
There was a problem hiding this comment.
I don't think this is accurate. When there's an empty array, it means the master flag is irrelevant, not that it's false. Generally that means there's only one, which may as well be master; so it would default to true. Or, better, it would remain unset entirely to be consistent with the other services.
There was a problem hiding this comment.
@Crell The last commit keeps the other cases (empty object, empty array, and omitted query) unset, only setting if the is_master key is present. Comparing the test run on query-fix and master, it looks like that unset properties (i.e. IsMaster bool) do default to their "zero values", which in this case is false.
For elasticsearch (empty object)
master:
elasticsearch:[{http dtsla3sy7euhc-master-7rqtwti elasticsearch elasticsearch.internal false 169.254.250.214 elasticsearch elasticsearch:5.4 9200 bwhgfsnjp7kzqlf7pd35dfg6mm.elasticsearch.service._.us-2.platformsh.site {false}}]
query-fix:
elasticsearch:[{http rjify4yjcwxaa-master-7rqtwti elasticsearch elasticsearch.internal false 169.254.49.124 elasticsearch elasticsearch:5.4 9200 j2dkzht3gs2yr66fb2brhoj4zu.elasticsearch.service._.eu-3.platformsh.site {false} map[]}]
There was a problem hiding this comment.
Here's the query omitted case (influxdb):
master:
influxdb:[{http rjify4yjcwxaa-master-7rqtwti influxdb influxdb.internal false 169.254.70.27 influxdb influxdb:1.3 8086 haz5rys6n2dsnjusqa54os3ii4.influxdb.service._.eu-3.platformsh.site {false}}]
query-fix:
influxdb:[{http rjify4yjcwxaa-master-7rqtwti influxdb influxdb.internal false 169.254.70.27 influxdb influxdb:1.3 8086 haz5rys6n2dsnjusqa54os3ii4.influxdb.service._.eu-3.platformsh.site {false} <nil>}] redis:[{redis rjify4yjcwxaa-master-7rqtwti redis redis.internal false 169.254.39.193 redis redis:5.0 6379 2wye4dawv22vhvozyec3o5hxfm.redis.service._.eu-3.platformsh.site {false} []}]
gohelper.go
Outdated
| rels[k][0].Query.IsMaster = val.(bool) | ||
| } else { | ||
| // Handle omitted case | ||
| rels[k][0].Query.IsMaster = false |
| } | ||
|
|
||
| func TestCredentialsForRelationshipPropertyQueryAbsentReturns(t *testing.T) { | ||
| config, err := psh.NewRuntimeConfigReal(helper.RuntimeEnv(psh.EnvList{}), "PLATFORM_") |
There was a problem hiding this comment.
I wonder if these tests would be more valid if they took an existing config and mutated it, rather than having them lying around as discrete test data? I'm honestly unsure here, nor do I know how much work that would be.
| } | ||
| RawQuery interface{} `json:"query"` |
There was a problem hiding this comment.
This should probably be rawQuery so that it's not accessible outside this package. It's an internal implementation detail.
In the
PLATFORM_RELATIONSHIPSarray, thequeryproperty is handled inconsistently.On postgresql, mongodb, and mysql, it is an object with a single property,
is_master. On redis and elasticsearch, it is an empty array. On rabbitmq, solr, memcached, and influxdb, it is omitted entirely.Previously, this returned the following error when parsing the JSON string:
This is a workaround that includes tests for the two proposed changes to the JSON relationships encoding, so it will not break when fixed:
queryproperty entirely where currently defined as an empty array (TestCredentialsForRelationshipPropertyQueryAbsentReturns).TestCredentialsForRelationshipPropertyQueryDefinedAsEmptyObjectReturns).Previously in cases where
querywas not defined at all,Credential.Query.IsMasterdefaulted to the type's (bool) zero value (false),so that has been replicated here in the empty array andquery-omitted cases.Otherwise, the value defined in
is_masteris used.