[SNOW-1882616] Error out for duplicate keys in variant #929
Conversation
| @@ -856,6 +856,29 @@ public void testValidateAndParseObject() throws Exception { | |||
| () -> validateAndParseObjectNew("COL", Collections.singletonMap("foo", new Object()), 0)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testValidateDuplicateKeys() { | |||
There was a problem hiding this comment.
Could we add a test that tests validateAndParseVariantNew, as well as validateAndParseArrayNew (array of objects with duplicated keys)?
| @@ -176,6 +177,10 @@ private static String validateAndParseSemiStructured( | |||
| throw valueFormatNotAllowedException( | |||
| columnName, snowflakeType, "Not a valid JSON", insertRowIndex); | |||
| } catch (IOException e) { | |||
| if (e.getMessage().contains("Duplicate field")) { | |||
| throw valueFormatNotAllowedException( | |||
There was a problem hiding this comment.
Let's 'chain' exceptions like you do below: pass the original e exception to the constructor of the one you throw. This will preserve information contained in the original exception.
I wonder if the "Not a valid JSON: duplicate field" message you're adding to the child exception becomes redundant in this case: the original one already has similar information. Keep or edit or delete, up to you.
There was a problem hiding this comment.
@sfc-gh-dorlovsky We don't want to preserve information here, it may lead to customer data being exposed in client side logs.
| if (e.getMessage().contains("Duplicate field")) { | ||
| throw valueFormatNotAllowedException( | ||
| columnName, snowflakeType, "Not a valid JSON: duplicate field", insertRowIndex); | ||
| } | ||
| throw new SFException(e, ErrorCode.IO_ERROR, "Cannot create JSON Parser or JSON generator"); |
There was a problem hiding this comment.
Nit: would it be useful to add columnName, snowflakeType, and insertRowIndex to this exception too?
The SDK should not ingest the variant like {"key": 1, "key": 2} otherwise, we would have issues in the XP during select and there is no good mitigation on that.
In this change, we would error out with INVALID_VALUE_ROW for such duplicate cases.
Added Unit test for the change.