EES-5112 - adding validation to ensure Candidate Keys are valid #5034
Conversation
duncan-at-hiveit
force-pushed
the
EES-5112-and-5115-add-validation-for-mapping-candidates
branch
3 times, most recently
from
59de0c9 to
e2cf678
Compare
duncan-at-hiveit
force-pushed
the
EES-5112-and-5115-add-validation-for-mapping-candidates
branch
from
e2cf678 to
d24739a
Compare
… a boolean for testing the existence of a CandidateKey rather than having to deserialise the Candidate just to test for its existence.
duncan-at-hiveit
force-pushed
the
EES-5112-and-5115-add-validation-for-mapping-candidates
branch
from
d24739a to
5097b35
Compare
| .Set<JsonBool>() | ||
| .FromSqlRaw( | ||
| sql: $""" | ||
| SELECT COALESCE("{request.JsonbColumnName}" #> @jsonPath, '[]'::jsonb) |
There was a problem hiding this comment.
is the , '[]'::jsonb part needed?
There was a problem hiding this comment.
Yup, this means that if {request.JsonbColumnName}" #> @jsonPath resolves to NULL, then this will fall back to a non-null JSON object for the key to be compared against by using "[]" instead of null.
Without this fallback, it goes kaboom!
There was a problem hiding this comment.
but doesn't COALESCE return the first non-null value in the parameter list? And if none are non-null, then it returns null itself? So I thought it would have the same behaviour?
There was a problem hiding this comment.
Yup, but given that "[]" is never null, the whole COALESCE will be guaranteed to return something, which is what's important here.
There was a problem hiding this comment.
oh, we're saying it's the ? @keyValue part which will blow up, unless we have that []? That makes sense.
Sorry, I thought we were trying to prevent the COALESCE blowing up
| { | ||
| "Target filter 1 option 2 key", new MappableFilterOption("Filter 1 option 2 label") | ||
| }, | ||
| { "Target filter 1 option 3 key", new MappableFilterOption("Filter 1 option 3 label") } |
There was a problem hiding this comment.
might be nice to format this the same as the ones above :)
There was a problem hiding this comment.
I've given this file a blanket reformat, which cleans this one up too!
| Level = GeographicLevel.Country, | ||
| SourceKey = "source-la-location-3-key", | ||
| Type = MappingType.ManualMapped, | ||
| CandidateKey = "target-la-location-1-key" |
There was a problem hiding this comment.
I'm not super fussed, but at first I didn't realise that the reason this candidate is invalid is because it is not a candidate for the Country level, even though it is a candidate for the LocalAuthority level.
This is a different failure scenario to the candidate key target-la-location-2-key above, which doesn't exist at all.
I just wondered if we should break these out into 2 separate tests to make it clearer - and because they could potentially go down different code paths? But I don't feel strongly either way
There was a problem hiding this comment.
I've gone for a middle ground if that's OK, and added comments to explain the different scenarios being tested. Done the same for the filter option equivalent too.
There was a problem hiding this comment.
Nice! Yep, I prefer this compromise actually :)
…ios being tested.
duncan-at-hiveit
deleted the
EES-5112-and-5115-add-validation-for-mapping-candidates
branch
This PR:
Location candidate validation
This validation is pretty simple. We only need to take the "Level" supplied in the update request, and see if there is a candidate Location within that same Level with the given CandidateKey.
Filter option validation
This validation is a little more involved. In order to validate, we:
New "KeyExistsAtJsonbPath" method
This is an optimised method that in this PR we are using to check for the existence of a key within a given chunk of JSON.