Fix not to render optional fields when unset #55

rjalander · 2023-05-24T09:34:43Z

This PR has the changes to fix the issue #51, not to render the optional fields when null during serialization using an ObjectMapper.

afrittoli · 2023-05-24T10:07:09Z

src/test/java/dev/cdevents/CDEventsTest.java

+        String cdEventJson = CDEvents.cdEventAsJson(cdEvent);
+
+        CloudEvent ceEvent = CDEvents.cdEventAsCloudEvent(cdEvent);
+
+        String ceDataJson = new String(ceEvent.getData().toBytes(), StandardCharsets.UTF_8);
+
+        assertThat(ceEvent.getType()).isEqualTo(cdEvent.getContext().getType());
+        assertThat(ceEvent.getSource()).isEqualTo(cdEvent.getContext().getSource());
+        assertThat(ceDataJson).isEqualTo(cdEventJson);


If I understand the test correctly, what it does it:

create a CDEvent object, renders it to json

create a CloudEvent out of the CDEvent

extract the payload from the CloudEvent and matches is against the CDEvent as json

compare type and source

This looks good to test check CloudEvent binding, but how does it help to test the unset fields?
Don't we need JSON documents as test data for that?

Yes, added test data to compare optional fields with the actual CDEvent.
To keep this PR smaller, added the optional field validation only for Core events for now, will create separate PRs to handle tests for other type of events.

afrittoli · 2023-05-26T10:13:40Z

src/test/java/dev/cdevents/CDEventsTest.java

+    @Test
+    void createTaskRunFinishedEventWithOptionalFieldsUnset() throws IOException {
+        InputStream inputStream = getClass().getResourceAsStream("/taskrun_finished_optional.json");
+        TaskRunFinishedCDEvent expectedCDEvent = objectMapper.readValue(inputStream, TaskRunFinishedCDEvent.class);


I think this can be ok, it guarantees consistent behaviour, however, I'm not 100% it guarantees that optional fields are not included in the rendered JSON.

At this level, I know that:

if I create an object without optional fields, those optional fields are not set in the object.

if I render that object without optional fields and parse it back, those optional fields are not set in the object.

If a load an object from JSON that does not specify optional fields, those optional fields are not set in the object

I'm still not 100% sure that the rendered JSON will not contain the optional fields set as well, but perhaps that's covered already by other tests - wdyt?

I see two possible ways we could test that further - possibly not for every event, just once might be enough.

either read bytes from inputStream and compare it with cdEventJson converted to bytes. This can be tricky since you may need to compact the json first (to avoid issues with spaces, indentations etc) and also ordering in dicts may be an issue unless the json rendered implementation always renders in the same order

or add one resource file with the optional fields set to empty value. Load it and check that the optional fields are in the object, with empty or null value.

Yes it compares the Subject's content object as whole but not field to filed comparison.
Or I can compare from both the objects that all optional fields are set to null and they are equal, something like;

assertThat(cdEvent.getSubject().getContent().getTaskName()).isEqualTo(null); assertThat(expectedCDEvent.getSubject().getContent().getTaskName()).isEqualTo(null);

Now updated the test validation by comparing the JsonNodes created from inputStream and cdEventJson
This way I can compare actual and expected optional and mandatory fields from the test data.

afrittoli · 2023-05-26T10:14:59Z

src/test/java/dev/cdevents/CDEventsTest.java

+        cdEvent.setSubjectPipelineRunId("/dev/pipeline/run/subject");
+
+        String cdEventJson = CDEvents.cdEventAsJson(cdEvent);
+        CloudEvent ceEvent = CDEvents.cdEventAsCloudEvent(cdEvent);


Do we really need to go through the cloudevent binding in this test?
It does not harm but I thought that would be covered already by previous tests.
It's fine to keep it if you deem it relevant here.

Yes to validate the CDEvent against schema, I need to invoke CDEvents.cdEventAsCloudEvent(cdEvent);

But I think I can move validation part to the CDEvents.cdEventAsJson(cdEvent); method itself as cdEventAsJson() is invoked from cdEventAsCloudEvent() to set the data in the CloudEvent. So that I can remove cloudevent binding from this test also.

Thanks for the clarification!

src/main/java/dev/cdevents/models/TaskRunStartedSubject.java

afrittoli · 2023-05-30T13:31:21Z

src/main/java/dev/cdevents/models/PipelineRunFinishedSubject.java

+
+            PipelineRunFinishedSubjectContent that = (PipelineRunFinishedSubjectContent) o;
+
+            if (getPipelineName() != null ? !getPipelineName().equals(that.getPipelineName()) : that.getPipelineName() != null) {


NIT: Since pipeline name is a string, this could be if (getPipelineName() == that.getPipelineName()) which would return true also in case they're both null

Now updated the test validation by comparing the JsonNodes, equals method is no longer needed.

afrittoli · 2023-05-31T14:45:26Z

src/main/java/dev/cdevents/CDEvents.java

@@ -36,6 +36,10 @@ private CDEvents() {
     */
    public static String cdEventAsJson(CDEvent cdEvent) {
        try {
+            if (!validateCDEvent(cdEvent)) {


Thanks - I think this is fine, perhaps in future, we might add a flag to make validation optional.
In the golang SDK I did not include validation in the JSON rendering, I made a validate method available instead, so that it is possible to render and invalid CDEvent to JSON, and the producer can decide whether to validate or not.

I did include a call to validate in the cloudevents binding to enforce sending of valid events thought (similar to what you had before).

I wonder whether instead of moving the validation from cdEventAsCloudEvent to cdEventAsJson, it would be better to add a call to validate in the test?

I'm also ok with keeping it as it is today.

Agree, even this keeps the behavior consistent.
Reverting to keep the validation only for CloudEvents binding, removed for JSON render and added test assertion to validate CDEvent against schema.

afrittoli

Thanks for the updates!

afrittoli

Thanks for the updates!

afrittoli · 2023-05-31T15:25:12Z

@rjalander is it ok if squash and merge now, or would you like more reviews from other Java SDK maintainers?

rjalander · 2023-05-31T15:30:20Z

I think we are good to merge, If you have no other comments.

cdevents-bot · 2023-07-24T13:46:40Z

🎉 This issue has been resolved in v0.1.2 (Release Notes)

Fix not to render optional fields when unset

de4afaf

rjalander requested a review from afrittoli May 24, 2023 09:59

afrittoli reviewed May 24, 2023

View reviewed changes

Jalander Ramagiri added 3 commits May 25, 2023 11:47

Adding CDEvent validation with test data optional fields unset

46dcddc

fixing linter issues and override hashCode

d47f9f0

fixing linter issues

04a08a1

afrittoli reviewed May 26, 2023

View reviewed changes

src/main/java/dev/cdevents/models/TaskRunStartedSubject.java Outdated Show resolved Hide resolved

rjalander requested a review from afrittoli May 30, 2023 12:48

afrittoli reviewed May 30, 2023

View reviewed changes

Jalander Ramagiri added 4 commits May 31, 2023 12:56

test validation with JsonNode

5320531

Undo changes as per master copy

eb71d25

removed org.json from test

ffe1f93

remove validation for JSON render and keep only for cloudevents binding

150084a

rjalander requested a review from afrittoli May 31, 2023 14:36

afrittoli reviewed May 31, 2023

View reviewed changes

afrittoli approved these changes May 31, 2023

View reviewed changes

afrittoli merged commit 1d42acc into cdevents:main May 31, 2023

rjalander mentioned this pull request Jun 1, 2023

Validating CDEvent fails if not setting Optional fields #51

Closed

cdevents-bot added the released Issue has been released label Jul 24, 2023

cdevents-bot added this to the v0.1.2 milestone Jul 24, 2023


		PipelineRunFinishedSubjectContent that = (PipelineRunFinishedSubjectContent) o;

		if (getPipelineName() != null ? !getPipelineName().equals(that.getPipelineName()) : that.getPipelineName() != null) {

Fix not to render optional fields when unset #55

Fix not to render optional fields when unset #55

Uh oh!

Conversation

rjalander commented May 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afrittoli left a comment

Choose a reason for hiding this comment

Uh oh!

afrittoli left a comment

Choose a reason for hiding this comment

Uh oh!

afrittoli commented May 31, 2023

Uh oh!

rjalander commented May 31, 2023

Uh oh!

cdevents-bot commented Jul 24, 2023

Uh oh!

Uh oh!