feat: Customizing Cloudevents validation #594

touchkey · 2023-10-05T13:12:40Z

No description provided.

pierDipi

I would be curious on the design of it and why this approach over some other alternatives that you could have considered

pierDipi · 2023-10-19T08:06:41Z

To clarify my comment why not having a Validator that is set on the Builder ?

public interface CloudEventValidator {
    void Validate(CloudEvent) throws CloudEventValidationException;
}


// ... builder classes
public class CloudEventBuilder {

    // ... omitted methods ...

    public CloudEventBuilder withValidator(CloudEventValidator validator) {
        this.validator = validator;
        return this;
    }


   // use validator (if set) in `build()`

touchkey · 2023-10-25T03:28:57Z

My approach explaination
Yes... The reason I went with this design is.. as mentioned in this issue, We wanted to add this as part of wrapper library... with some default validation for our organization. These wrapper library will be given to our customers and they don't have to add validation from their side..
With this approach, we are creating a Validator class from our side(platform side) so that when we release this, our customers don't have to use Validators on their own as this generalized in our org over all teams.

Suggested approach explaination
Even I thought about adding this as a builder as you suggested, but these builders has to be set by the customer(initialize and add it to builder method) which we wanted to avoid for easy usage as we are a platform team who customizes the user experiences and releases libraries inside our org.

pierDipi · 2023-11-16T08:23:40Z

I'm not fully convinced on the approach, did you consider using something like SPI https://docs.oracle.com/javase/tutorial/ext/basics/spi.html ?

touchkey · 2024-01-31T04:37:58Z

Addressed review comment

pierDipi

Thanks!

Can we add new tests which would test end to end the provider ?

pierDipi · 2024-01-31T10:28:47Z

core/src/main/java/io/cloudevents/core/provider/CloudEventValidatorProvider.java

+/*
+ * Copyright 2018-Present The CloudEvents Authors
+ * <p>
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package io.cloudevents.core.provider;
+
+import io.cloudevents.CloudEvent;
+import io.cloudevents.core.validator.CloudEventValidator;
+
+import java.util.Iterator;
+import java.util.ServiceConfigurationError;
+import java.util.ServiceLoader;
+
+/**
+ * CloudEventValidatorProvider is a singleton class which loads and access CE Validator service providers on behalf of service clients.
+ */
+public class CloudEventValidatorProvider {
+
+    private static CloudEventValidatorProvider cloudEventValidatorProvider;
+
+    private ServiceLoader<CloudEventValidator> loader;
+
+    private CloudEventValidatorProvider(){
+        loader = ServiceLoader.load(CloudEventValidator.class);
+    }
+
+    public static synchronized CloudEventValidatorProvider getInstance(){
+        if(cloudEventValidatorProvider == null){
+            cloudEventValidatorProvider = new CloudEventValidatorProvider();
+        }
+        return cloudEventValidatorProvider;
+    }
+
+    /**
+     * iterates through available Cloudevent validators.
+     * @param cloudEvent
+     */
+    public void validate(CloudEvent cloudEvent){
+        try{
+            //
+            Iterator<CloudEventValidator> validatorIterator = loader.iterator();
+            while (validatorIterator.hasNext()){
+                CloudEventValidator validator = validatorIterator.next();
+                validator.validate(cloudEvent);
+
+            }
+        } catch (ServiceConfigurationError serviceError) {
+
+            serviceError.printStackTrace();
+        }
+
+    }
+
+
+
+
+
+}


I think we can:

remove the need of synchronize every time in build()

instead of catching and printing errors which could hide important mis-configurations, let the caller decide how to handle them

remove many empty lines

use more finals fields/variables to be explicit

Suggested change

/*

* Copyright 2018-Present The CloudEvents Authors

* 

* Licensed under the Apache License, Version 2.0 (the "License");

* you may not use this file except in compliance with the License.

* You may obtain a copy of the License at

* 

* http://www.apache.org/licenses/LICENSE-2.0

* 

* Unless required by applicable law or agreed to in writing, software

* distributed under the License is distributed on an "AS IS" BASIS,

* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

* See the License for the specific language governing permissions and

* limitations under the License.

*

*/

package io.cloudevents.core.provider;

import io.cloudevents.CloudEvent;

import io.cloudevents.core.validator.CloudEventValidator;

import java.util.Iterator;

import java.util.ServiceConfigurationError;

import java.util.ServiceLoader;

/**

* CloudEventValidatorProvider is a singleton class which loads and access CE Validator service providers on behalf of service clients.

*/

public class CloudEventValidatorProvider {

private static CloudEventValidatorProvider cloudEventValidatorProvider;

private ServiceLoader<CloudEventValidator> loader;

private CloudEventValidatorProvider(){

loader = ServiceLoader.load(CloudEventValidator.class);

}

public static synchronized CloudEventValidatorProvider getInstance(){

if(cloudEventValidatorProvider == null){

cloudEventValidatorProvider = new CloudEventValidatorProvider();

}

return cloudEventValidatorProvider;

}

/**

* iterates through available Cloudevent validators.

* @param cloudEvent

*/

public void validate(CloudEvent cloudEvent){

try{

//

Iterator<CloudEventValidator> validatorIterator = loader.iterator();

while (validatorIterator.hasNext()){

CloudEventValidator validator = validatorIterator.next();

validator.validate(cloudEvent);

}

} catch (ServiceConfigurationError serviceError) {

serviceError.printStackTrace();

}

}

}

/*

* Copyright 2018-Present The CloudEvents Authors

* 

* Licensed under the Apache License, Version 2.0 (the "License");

* you may not use this file except in compliance with the License.

* You may obtain a copy of the License at

* 

* http://www.apache.org/licenses/LICENSE-2.0

* 

* Unless required by applicable law or agreed to in writing, software

* distributed under the License is distributed on an "AS IS" BASIS,

* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

* See the License for the specific language governing permissions and

* limitations under the License.

*

*/

package io.cloudevents.core.provider;

import io.cloudevents.CloudEvent;

import io.cloudevents.core.validator.CloudEventValidator;

import java.util.ServiceLoader;

/**

* CloudEventValidatorProvider is a singleton class which loads and access CE Validator service providers on behalf of service clients.

*/

public class CloudEventValidatorProvider {

private static final CloudEventValidatorProvider cloudEventValidatorProvider = new CloudEventValidatorProvider();

private final ServiceLoader<CloudEventValidator> loader;

private CloudEventValidatorProvider() {

loader = ServiceLoader.load(CloudEventValidator.class);

}

public static CloudEventValidatorProvider getInstance() {

return cloudEventValidatorProvider;

}

/**

* iterates through available Cloudevent validators.

*

* @param cloudEvent event to validate.

*/

public void validate(final CloudEvent cloudEvent) {

for (final CloudEventValidator validator : loader) {

validator.validate(cloudEvent);

}

}

}

pierDipi · 2024-01-31T10:30:49Z

core/src/main/java/io/cloudevents/core/v1/CloudEventBuilder.java

@@ -119,7 +121,12 @@ public CloudEvent build() {
            throw createMissingAttributeException(TYPE);
        }

-        return new CloudEventV1(id, source, type, datacontenttype, dataschema, subject, time, this.data, this.extensions);
+        CloudEvent cloudEvent = new CloudEventV1(id, source, type, datacontenttype, dataschema, subject, time, this.data, this.extensions);


Suggested change

CloudEvent cloudEvent = new CloudEventV1(id, source, type, datacontenttype, dataschema, subject, time, this.data, this.extensions);

final CloudEvent cloudEvent = new CloudEventV1(id, source, type, datacontenttype, dataschema, subject, time, this.data, this.extensions);

pierDipi · 2024-01-31T10:35:12Z

core/src/main/java/io/cloudevents/core/v1/CloudEventBuilder.java

-        return new CloudEventV1(id, source, type, datacontenttype, dataschema, subject, time, this.data, this.extensions);
+        CloudEvent cloudEvent = new CloudEventV1(id, source, type, datacontenttype, dataschema, subject, time, this.data, this.extensions);
+
+        CloudEventValidatorProvider validator = CloudEventValidatorProvider.getInstance();


Suggested change

CloudEventValidatorProvider validator = CloudEventValidatorProvider.getInstance();

final CloudEventValidatorProvider validator = CloudEventValidatorProvider.getInstance();

pierDipi · 2024-01-31T10:36:04Z

core/src/main/java/io/cloudevents/core/v1/CloudEventBuilder.java

+        CloudEvent cloudEvent = new CloudEventV1(id, source, type, datacontenttype, dataschema, subject, time, this.data, this.extensions);
+
+        CloudEventValidatorProvider validator = CloudEventValidatorProvider.getInstance();
+        validator.validate(cloudEvent);


We need to add the same logic for the 0.3 builder https://github.com/cloudevents/sdk-java/blob/main/core/src/main/java/io/cloudevents/core/v03/CloudEventBuilder.java

pierDipi · 2024-01-31T10:37:07Z

See also DCO error here https://github.com/cloudevents/sdk-java/pull/594/checks?check_run_id=21047384482

touchkey · 2024-02-07T06:18:03Z

Review comment addressed

pierDipi

The new test is failing

pierDipi · 2024-02-07T08:26:40Z

README.md

+If there is a security concern with one of the CloudEvents specifications, or
+with one of the project's SDKs, please send an email to
+[cncf-cloudevents-security@lists.cncf.io](mailto:cncf-cloudevents-security@lists.cncf.io).
+


This seems an unrelated change ?

I did a rebase while doing DCO change. I think after that it came

pierDipi · 2024-02-08T09:00:07Z

Just to clarify/help fixing the test.

we would need to implement the SPI for testing, see this https://www.baeldung.com/java-spi, in particular the section on "Building the Service Provider" that part should be done in the test directory, an example is also in the SDK here https://github.com/cloudevents/sdk-java/blob/main/core/src/test/resources/META-INF/services/io.cloudevents.core.format.EventFormat for testing the EventFormat

touchkey · 2024-02-15T05:40:53Z

Just to clarify/help fixing the test.

we would need to implement the SPI for testing, see this https://www.baeldung.com/java-spi, in particular the section on "Building the Service Provider" that part should be done in the test directory, an example is also in the SDK here https://github.com/cloudevents/sdk-java/blob/main/core/src/test/resources/META-INF/services/io.cloudevents.core.format.EventFormat for testing the EventFormat

I had this commit in an unversioned commit. Now committed those files. Apologies for missing

Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

Signed-off-by: Doug Davis <dug@microsoft.com> Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

Signed-off-by: Calum Murray <cmurray@redhat.com> Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

pierDipi

LGTM

Add SPI for custom CloudEvent validation. Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

touchkey marked this pull request as draft October 6, 2023 02:22

touchkey force-pushed the cloudevents-validation branch from d2706e0 to 8389a27 Compare October 6, 2023 05:14

touchkey marked this pull request as ready for review October 6, 2023 05:14

pierDipi reviewed Oct 17, 2023

View reviewed changes

pierDipi reviewed Jan 31, 2024

View reviewed changes

touchkey force-pushed the cloudevents-validation branch from 43ea478 to 1beaf00 Compare February 7, 2024 04:54

pierDipi reviewed Feb 7, 2024

View reviewed changes

vbhat6 and others added 6 commits February 15, 2024 11:25

feat: Customizing Cloudevents validation

13de314

Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

Add link to CloudEvent security mailing list (cloudevents#599)

4588166

Signed-off-by: Doug Davis <dug@microsoft.com> Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

feat: Customizing Cloudevents validation

977e846

Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

feat: Customizing Cloudevents validation

c92553c

Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

Update cesql TCK tests (cloudevents#603)

62c55e7

Signed-off-by: Calum Murray <cmurray@redhat.com> Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

feat: Customizing Cloudevents validation

8c66b1b

Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

touchkey force-pushed the cloudevents-validation branch from ea6b839 to 8c66b1b Compare February 15, 2024 05:55

Merge branch 'cloudevents:main' into cloudevents-validation

2cc7b78

pierDipi approved these changes Feb 15, 2024

View reviewed changes

pierDipi merged commit 111fb55 into cloudevents:main Feb 15, 2024
5 checks passed

lyca pushed a commit to lyca/cloudevents-sdk-java that referenced this pull request Feb 17, 2024

feat: Customizing Cloudevents validation (cloudevents#594)

f87a0dc

Add SPI for custom CloudEvent validation. Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Customizing Cloudevents validation #594

feat: Customizing Cloudevents validation #594

touchkey commented Oct 5, 2023

pierDipi left a comment

pierDipi commented Oct 19, 2023

touchkey commented Oct 25, 2023

pierDipi commented Nov 16, 2023

touchkey commented Jan 31, 2024

pierDipi left a comment

pierDipi Jan 31, 2024

pierDipi Jan 31, 2024

pierDipi Jan 31, 2024

pierDipi Jan 31, 2024

pierDipi commented Jan 31, 2024

touchkey commented Feb 7, 2024

pierDipi left a comment

pierDipi Feb 7, 2024

touchkey Feb 7, 2024

pierDipi commented Feb 8, 2024

touchkey commented Feb 15, 2024

pierDipi left a comment

	CloudEvent cloudEvent = new CloudEventV1(id, source, type, datacontenttype, dataschema, subject, time, this.data, this.extensions);
	final CloudEvent cloudEvent = new CloudEventV1(id, source, type, datacontenttype, dataschema, subject, time, this.data, this.extensions);

	CloudEventValidatorProvider validator = CloudEventValidatorProvider.getInstance();
	final CloudEventValidatorProvider validator = CloudEventValidatorProvider.getInstance();

feat: Customizing Cloudevents validation #594

feat: Customizing Cloudevents validation #594

Conversation

touchkey commented Oct 5, 2023

pierDipi left a comment

Choose a reason for hiding this comment

pierDipi commented Oct 19, 2023

touchkey commented Oct 25, 2023

pierDipi commented Nov 16, 2023

touchkey commented Jan 31, 2024

pierDipi left a comment

Choose a reason for hiding this comment

pierDipi Jan 31, 2024

Choose a reason for hiding this comment

pierDipi Jan 31, 2024

Choose a reason for hiding this comment

pierDipi Jan 31, 2024

Choose a reason for hiding this comment

pierDipi Jan 31, 2024

Choose a reason for hiding this comment

pierDipi commented Jan 31, 2024

touchkey commented Feb 7, 2024

pierDipi left a comment

Choose a reason for hiding this comment

pierDipi Feb 7, 2024

Choose a reason for hiding this comment

touchkey Feb 7, 2024

Choose a reason for hiding this comment

pierDipi commented Feb 8, 2024

touchkey commented Feb 15, 2024

pierDipi left a comment

Choose a reason for hiding this comment