Feature/structured metadata #171
Conversation
…, more complete tests.
|
@nitzanj , one of the tests failed. please check |
|
|
||
| /** | ||
| * Update the datasource entries for a given field | ||
| * @param fieldExternalId The id of the field to update |
There was a problem hiding this comment.
Inconsistency - sometimes it is called externalId and sometimes fieldExternalId.
I suggest using fieldId (not sure what the external represents)
There was a problem hiding this comment.
When you send an ID to a function about a field you an omit the 'field' for brevity. When you need to provide an id for a field in the context of another API (e.g. datasource, that has it's own id() you must include the 'field' in the parameter name for clarity.
I can use 'fieldExternalId' everywhere for consistency. The word external is part of the spec, that's the key used in all the requests and responses, we have to use it.
There was a problem hiding this comment.
Regarding the fieldExternalId vs. externalId, got it.
Regarding the external,
I understand that this is what the server is using, but I don't understand why they are using it.
In any case, why do we have to be aligned with the server naming?
There was a problem hiding this comment.
I had a long discussion about it with docs, for them it's real bad if the API docs use different names than the SDK docs so we're using the same names. We can open it up tomorrow to if you want, for us it's a quick easy fix...
There was a problem hiding this comment.
Cool. let's talk about it
cloudinary-test-common/src/main/java/com/cloudinary/test/AbstractStructuredMetadataTest.java
Outdated
Show resolved
Hide resolved
| addFieldToAccount(stringField); | ||
|
|
||
| ApiResponse result = cloudinary.api().listMetadataFields(); | ||
| assertNotNull(result); |
There was a problem hiding this comment.
I can assert that there's a list, not the actual item as it may be far behind and not get fetched on the first batch (parallel testing).
There was a problem hiding this comment.
Not sure I get what you mean...
| ApiResponse fieldResult = addFieldToAccount(newFieldInstance("testDeleteField")); | ||
| ApiResponse result = api.deleteMetadataField(fieldResult.get("external_id").toString()); | ||
| assertNotNull(result); | ||
| assertEquals("ok", result.get("message")); |
There was a problem hiding this comment.
Check that getMetadataField returns null
There was a problem hiding this comment.
Why? Java tests are slow as it is, and subsequent calls are 100% server tests with no implications on the SDK whatsoever. Since we do assert the server ACK, checking again will turn this into a pure backend regression test...
| MetadataDataSource.Entry newEntry = new MetadataDataSource.Entry("id1", "new1"); | ||
| ApiResponse result = api.updateMetadataFieldDatasource(fieldResult.get("external_id").toString(), Collections.singletonList(newEntry)); | ||
| assertNotNull(result); | ||
| assertEquals("new1", ((Map) ((List) result.get("values")).get(0)).get("value")); |
There was a problem hiding this comment.
Check what happens when adding an entry that already exists
There was a problem hiding this comment.
Again, your call but I was trying to avoid making this a full backend integration/regression test. There isn't even a 'correct' behaviour in the spec to test against...
| Map<String, Object> metadata = Collections.<String, Object>singletonMap(fieldId, "123456"); | ||
| Map result = cloudinary.uploader().upload(SRC_TEST_IMAGE, asMap("metadata", metadata, "tags", Arrays.asList(SDK_TEST_TAG, METADATA_UPLOADER_TAG))); | ||
| assertNotNull(result.get("metadata")); | ||
| assertEquals("123456", ((Map) result.get("metadata")).get(fieldId)); |
There was a problem hiding this comment.
What about testing validations? or sending wrong values?
No description provided.