feat: Customizing Cloudevents validation

[touchkey]

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]

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]

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]

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]

Addressed review comment

[pierDipi]

Thanks!

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

[pierDipi]

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
	* <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();
	}
	}
	}
	/*
	* 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.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]

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]

Suggested change

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

[pierDipi]

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]

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

[touchkey]

Review comment addressed

[pierDipi]

The new test is failing

[pierDipi]

This seems an unrelated change ?

[touchkey]

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

[pierDipi]

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]

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

[pierDipi]

LGTM