Support for creating Custom Event Types with schemas validation #83

rjalander · 2024-07-03T16:26:40Z

Changes to add APIs support to create customCDEventAsCloudEvent and customCDEventFromJson
also includes the validation of an official custom schema.json

Note:
Custom event support as per SDK support
Created sample CustomResourceCDEvent.java to test APIs as per example

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

aalmiray · 2024-07-04T19:55:35Z

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

-        String eventType = getUnVersionedEventTypeFromJson(cdEventJson);
-        CDEventConstants.CDEventTypes cdEventType = getCDEventTypeEnum(eventType);
+    public static <T extends CDEvent> CloudEvent customCDEventAsCloudEvent(T customCDEvent, boolean validateContextSchema) {
+        Optional.of(customCDEvent)


I'm not so sure about the use of Optional here instead of a null and validation check

yeah make sense

aalmiray · 2024-07-04T19:56:47Z

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

+     * @return valid cdEvent
+     */
+    public static boolean validateWithContextSchemaUri(CDEvent customCDEvent) {
+        return Optional.ofNullable(customCDEvent.customSchemaUri())


Same as previous comment on Optional

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

xibz

@rjalander Overall looks good. Most comments are fairly minor with one suggestion of how validation should return errors to be populated in the exception message (even add a CDEventsValidationException

xibz · 2024-07-08T22:20:19Z

sdk/src/test/java/dev/cdevents/custom/resource/Customresourcecreated.java

+    "customDataContentType"
+})
+@Generated("jsonschema2pojo")
+public class Customresourcecreated {


This is a very odd class name. The lack of camel casing I mean? I know it's a test class, but just seems odd

yeah noticed, but this is generated from jsonschema2pojo
Anyway removed this class as per the latest approach.

xibz · 2024-07-08T22:24:22Z

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

+
+    private static CloudEvent buildCloudEvent(CDEvent cdEvent) throws URISyntaxException {
+        String cdEventJson = cdEventAsJson(cdEvent);
+        log.info("CDEvent with type {} as json - {}", cdEvent.currentCDEventType(), cdEventJson);


Hmm, I wonder if this should be a debug log with the raw json in there. It seems overkill with it for info. How do you feel about changing this to log.debug for the JSON and having the log.info just having the CDEvent with type {}

xibz · 2024-07-11T13:34:15Z

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

        String eventType = getUnVersionedEventTypeFromJson(cdEventJson);
        CDEventConstants.CDEventTypes cdEventType = getCDEventTypeEnum(eventType);
        try {
-            CDEvent cdEvent = (CDEvent) new ObjectMapper().readValue(cdEventJson, cdEventType.getEventClass());
+            CDEvent cdEvent = new ObjectMapper().readValue(cdEventJson, cdEventType.getEventClass());


Why are we constructing a new object mapper every time?
Is this method only ever called once? Even so we probably should move it to a field

Removed this method using the existing APIs

xibz · 2024-07-11T13:35:44Z

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

-        CDEventConstants.CDEventTypes cdEventType = getCDEventTypeEnum(eventType);
+    public static <T extends CDEvent> CloudEvent customCDEventAsCloudEvent(T customCDEvent, boolean validateContextSchema) {
+        if (!validateCustomCDEvent(customCDEvent, validateContextSchema)) {
+            throw new CDEventsException("Custom CDEvent validation failed.");


Do we not say why something has failed in the exception? We should probably create an issue for that if that's the case.

the method with in the condition logs the reason for failures before the exception is thrown.

xibz · 2024-07-11T13:36:06Z

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

+     */
+    public static <T extends CDEvent> T customCDEventFromJson(String customCDEventJson, Class<T> eventClass, boolean validateContextSchema) {
+        try {
+            T cdEvent = new ObjectMapper().readValue(customCDEventJson, eventClass);


Some comment here with the object mapper. We can just move this to a static field

Removed this method now and using it from existing code

xibz · 2024-07-11T13:36:21Z

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

+    public static <T extends CDEvent> T customCDEventFromJson(String customCDEventJson, Class<T> eventClass, boolean validateContextSchema) {
+        try {
+            T cdEvent = new ObjectMapper().readValue(customCDEventJson, eventClass);
+            if (!validateCustomCDEvent(cdEvent, validateContextSchema)) {


Same comment above with the reasons something has failed

Using an existing method now, and added more logging for the failure reason

xibz · 2024-07-11T13:37:23Z

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

+     * @param customCDEvent
+     * @return valid cdEvent
+     */
+    public static boolean validateWithOfficialCustomSchema(CDEvent customCDEvent) {


xibz · 2024-07-11T13:44:38Z

sdk/src/test/java/dev/cdevents/custom/CustomCDEventsTest.java

+    void testCustomResourceEventFromJsonConformance() throws IOException {
+
+        // Use spec/custom/conformance.json to test once Links are implemented for SDK
+        //File customResourceCreatedExample = new File(CUSTOM_SPEC_FOLDER + "conformance.json");


Suggested change

//File customResourceCreatedExample = new File(CUSTOM_SPEC_FOLDER + "conformance.json");

// File customResourceCreatedExample = new File(CUSTOM_SPEC_FOLDER + "conformance.json");

xibz · 2024-07-11T13:44:57Z

sdk/src/test/java/dev/cdevents/custom/CustomCDEventsTest.java

+        cdEvent.setSubjectNestedList(Arrays.asList("val1", "val2"));
+
+        cdEvent.setCustomSchemaUri(new File("src/test/resources/customresourcecreated.json").toURI());
+


nit: This spacing seems really odd

afrittoli

Thanks @rjalander - I've not reviewed the whole code, but I have a comment about downloading the schema provided in an event, please let me know what you think about it.

On golang side, I have not added validation based on schemaUri yet, I'm working on it. I wanted to do it as a separate PR because schemaUri applies to all events, custom and not.

afrittoli · 2024-07-11T15:28:01Z

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

+        }
+        log.info("Validate custom CDEvent against context.schemaUri - {}", customCDEvent.customSchemaUri());
+        JsonSchemaFactory factory = JsonSchemaFactory.getInstance(SpecVersion.VersionFlag.V202012);
+        JsonSchema jsonSchema = factory.getSchema(customCDEvent.customSchemaUri());


I think we should not promote a model where for every message the SDK reaches out to some internet location to fetch the schema. The way I was thinking of implementing this for the sdk-go is to let user register a custom schema, so that it's loaded once and re-used from memory, and then give users an option to decide what to do if the custom schemaURI is not in the in-memory schema cache, i.e. fail or try to download the schema, with the former as default.

yes agree, will do the required changes in a separate PR. Removed this implementation from current PR and created issue to work on #84

afrittoli · 2024-07-11T15:29:15Z

generator/src/main/resources/template/event-template.mustache

+    * @return custom schema URI
+    */
+    @Override
+    public URI customSchemaUri() {


NIT: the field name is schemaUri - perhaps the method name should match it for clarity?

changing it to contextSchemaUri()

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

rjalander · 2024-07-15T10:10:55Z

Thank you @aalmiray @xibz @afrittoli for reviewing this PR
Initially I have created custom CDEvents support using additional APIs customCDEventAsCloudEvent and customCDEventFromJson assuming that various custom events will be created by user of the SDK by extending CDEvent model.
But after looking at the approach from go-sdk, generating a base CustomTypeEvent for all the custom event types and using existing APIs to create CloudEvent and Custom events.

Please review the latest changes.
Will create a doc example to be inline with go-sdk as per https://github.com/cdevents/sdk-go/tree/main/docs#create-a-custom-cdevent

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

docs/CUSTOM_CDEVENTS.md

afrittoli · 2024-07-16T20:15:37Z

sdk/src/main/java/dev/cdevents/events/CustomTypeEvent.java

+// Code generated by dev.cdevents.generator.CDEventsGenerator. DO NOT EDIT.
+
+/*
+Copyright 2023 The CDEvents Authors


Actually it is generated from a common template for all the existing and new events.

xibz

Most of my comments are nit, but I had one thing that I think we should change. Doesn't have to be this PR, but a follow up one maybe after some discussion about it in CDEvents

Otherwise looks good! Since none of my comments are blocking, approved!

xibz · 2024-07-20T13:41:34Z

docs/CUSTOM_CDEVENTS.md

+</dependency>
+```
+### Create a Custom CDEvent
+Custom CDEvent can be created using a class `CustomTypeEvent` packaged with in `cdevents-sdk-java`


This reads very similarly to the sentence above:

Custom events follow the CDEvents format and can be defined via the
CustomTypeEvent class, available since v0.4.

How about changing the wording here to be something more like

In this example, we will create a custom event for our tool utilizing the new `CustomTypeEvent`

xibz · 2024-07-20T17:14:56Z

generator/src/main/java/dev/cdevents/generator/CDEventsGenerator.java

        log.info("Processing event JsonSchema file: {}", file.getAbsolutePath());
        try {
            JsonNode rootNode = objectMapper.readTree(file);
            JsonNode contextNode = rootNode.get("properties").get("context").get("properties");
            JsonNode subjectNode = rootNode.get("properties").get("subject").get("properties");
            String schemaURL = rootNode.get("$id").asText();
+            boolean isCustomEvent = schemaURL.endsWith("custom");


This seems really fragile. So any URL that ends with custom is assumed to be custom? Do we define that anywhere? Basically if we have some other even name "some/spec/that-has-custom" would be skipped is it wasn't a custom event

While concerning, and I dont think it needs to be addressed in this PR, can you please add a comment or a TODO to make it less fragile? This may require work in the spec SIG to discuss how to easily identify custom events based on URL.

No, not looking for custom event schema to check isCustomEvent here, generating a base CustomTypeEvent.java from an official schema.json, which has "$id": "https://cdevents.dev/0.5.0-draft/schema/custom",.
And this class file will be used to create any custom events

We discussed this in the CDEvents WG and agreed that extending the match to schema/custom should be sufficient. WDYT?

yeah sounds good to me.

xibz · 2024-07-20T17:15:46Z

generator/src/main/java/dev/cdevents/generator/CDEventsGenerator.java

-            updateSubjectContentProperties(schemaData, subjectContentNode);
+            schemaData.setCustomEvent(isCustomEvent);
+
+            if (!isCustomEvent) {


nit: We can probably just this up some and inverse the if statement

if (isCustomEvent) { return schemaData; }

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

rjalander · 2024-07-24T15:26:55Z

@aalmiray @afrittoli could you please review this PR and approve If there are no further comments.

Jalander Ramagiri added 14 commits April 29, 2024 14:28

updating spec gitmodule to v0.4.0

ddd02fb

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Generate SDK with spec 0.4.1

0b345f8

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Add module preprocessor and fix linter issues

5a2f5fc

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

preprocessor fix linter issues

0d0c1c0

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

adding preprocessor package-info

0f591e6

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

fixing review comments

e32f8c5

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Update README and cleanup

51b3ffc

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Update with review comments

88d3bdd

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Update PreprocessSchemas with interface implementation

bbd4279

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Fix PreprocessSchemas linting issues

5309a0f

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Update README to locate PreprocessSchemas

03f986b

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

adding custom event APIs support and validation

9ff4519

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

correcting setChainId method argument

b2c37fd

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

merge from main branch

aab785a

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

rjalander requested review from aalmiray and afrittoli July 3, 2024 16:27

Jalander Ramagiri added 2 commits July 4, 2024 11:27

fix linter issues

67d2b79

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

fix linter issues

0bb99f2

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

rjalander requested a review from xibz July 4, 2024 13:36

aalmiray reviewed Jul 4, 2024

View reviewed changes

addressing review comments

65856c9

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

xibz requested changes Jul 11, 2024

View reviewed changes

afrittoli reviewed Jul 11, 2024

View reviewed changes

creating a baseEvent for custom events

220cb9f

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

rjalander requested a review from adamkenihan July 15, 2024 08:34

rjalander mentioned this pull request Jul 15, 2024

schema validation using context schemaUri #84

Open

fix linter issues

cadb9a7

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Jalander Ramagiri added 2 commits July 15, 2024 11:50

fix linter and add isCustomEvent method

d737489

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

removing unused file

c8ffb25

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Jalander Ramagiri added 3 commits July 15, 2024 13:36

fix linter issues in SchemaData.java

6ce4bad

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

fix linter issues in SchemaData.java

495c0b3

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

adding doc CUSTOM_CDEVENTS

32846cc

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

afrittoli reviewed Jul 16, 2024

View reviewed changes

docs/CUSTOM_CDEVENTS.md Show resolved Hide resolved

afrittoli reviewed Jul 16, 2024

View reviewed changes

rjalander requested review from aalmiray, afrittoli and xibz July 19, 2024 10:14

adamkenihan approved these changes Jul 19, 2024

View reviewed changes

xibz approved these changes Jul 20, 2024

View reviewed changes

Jalander Ramagiri added 2 commits July 23, 2024 10:30

update with review comments

dff52e9

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

updating isCustomEvent check

2227b6e

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

	//File customResourceCreatedExample = new File(CUSTOM_SPEC_FOLDER + "conformance.json");
	// File customResourceCreatedExample = new File(CUSTOM_SPEC_FOLDER + "conformance.json");

		cdEvent.setSubjectNestedList(Arrays.asList("val1", "val2"));

		cdEvent.setCustomSchemaUri(new File("src/test/resources/customresourcecreated.json").toURI());

Support for creating Custom Event Types with schemas validation #83

Are you sure you want to change the base?

Support for creating Custom Event Types with schemas validation #83

Conversation

rjalander commented Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xibz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xibz Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjalander commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xibz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjalander commented Jul 24, 2024

rjalander commented Jul 3, 2024 •

edited

Loading

xibz Jul 8, 2024 •

edited

Loading