Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Customizing Cloudevents validation #594

Merged
merged 7 commits into from
Feb 15, 2024

Conversation

touchkey
Copy link
Contributor

@touchkey touchkey commented Oct 5, 2023

No description provided.

@touchkey touchkey marked this pull request as draft October 6, 2023 02:22
@touchkey touchkey marked this pull request as ready for review October 6, 2023 05:14
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pierDipi
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

Addressed review comment

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

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

Comment on lines 1 to 52
/*
* 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();
}

}





}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}
}
}

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 124 to 127
CloudEvent cloudEvent = new CloudEventV1(id, source, type, datacontenttype, dataschema, subject, time, this.data, this.extensions);

CloudEventValidatorProvider validator = CloudEventValidatorProvider.getInstance();
validator.validate(cloudEvent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pierDipi
Copy link
Member

@touchkey
Copy link
Contributor Author

touchkey commented Feb 7, 2024

Review comment addressed

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test is failing

README.md Outdated
Comment on lines 111 to 114
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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems an unrelated change ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pierDipi
Copy link
Member

pierDipi commented Feb 8, 2024

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
Copy link
Contributor Author

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

vbhat6 and others added 6 commits February 15, 2024 11:25
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: 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>
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pierDipi 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
Add SPI for custom CloudEvent validation.

Signed-off-by: vbhat6 <vinayas_bhat@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants