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

Smart Charging - SetChargingProfile object and data types #26

Merged
merged 12 commits into from
Feb 8, 2024

Conversation

LiorBenAri
Copy link
Contributor

No description provided.

@YuriyDurov
Copy link
Member

Hello @LiorBenAri! This PR looks great, I just have a small issue with a part of it:

OCPI.Net uses contract validators for validating the presense of properties instead of marking properties as required in the contract model. Normally, we define all contract properties as nullable and then use contract validators to check whether a property was null or not. This approach allows us to better distinguish between null vs default values (like 0 for numbers) in incoming data, and also allows us to better form validation error messages in case if some properties did not satisfy the protocol requirements. It also allows us to better validate things like arrays (at least one item present, etc.) and define other specific validation rules.

Can we have this PR updated so that it would not define required properties in contract models but would use validators to check for null values instead?

Here is some OCPI.Net documentation on this topic

And Validators code is located under src/OCPI.Net.Validation/Validators

@LiorBenAri
Copy link
Contributor Author

Hello @LiorBenAri! This PR looks great, I just have a small issue with a part of it:

OCPI.Net uses contract validators for validating the presense of properties instead of marking properties as required in the contract model. Normally, we define all contract properties as nullable and then use contract validators to check whether a property was null or not. This approach allows us to better distinguish between null vs default values (like 0 for numbers) in incoming data, and also allows us to better form validation error messages in case if some properties did not satisfy the protocol requirements. It also allows us to better validate things like arrays (at least one item present, etc.) and define other specific validation rules.

Can we have this PR updated so that it would not define required properties in contract models but would use validators to check for null values instead?

Here is some OCPI.Net documentation on this topic

And Validators code is located under src/OCPI.Net.Validation/Validators

Hi Yuri,

Will edit as you requested.

Lior

@LiorBenAri
Copy link
Contributor Author

LiorBenAri commented Feb 8, 2024

Hello @LiorBenAri! This PR looks great, I just have a small issue with a part of it:
OCPI.Net uses contract validators for validating the presense of properties instead of marking properties as required in the contract model. Normally, we define all contract properties as nullable and then use contract validators to check whether a property was null or not. This approach allows us to better distinguish between null vs default values (like 0 for numbers) in incoming data, and also allows us to better form validation error messages in case if some properties did not satisfy the protocol requirements. It also allows us to better validate things like arrays (at least one item present, etc.) and define other specific validation rules.
Can we have this PR updated so that it would not define required properties in contract models but would use validators to check for null values instead?
Here is some OCPI.Net documentation on this topic
And Validators code is located under src/OCPI.Net.Validation/Validators

Hi Yuri,

Will edit as you requested.

Lior

I see that there isn't a validation logic for all of the library objects (for example Session\CDRs objects).
Do you consider adding validation logic for the ChargingProfile objects as mandatory?
Anyway we won't use the OcpiValidate mechanism because it doesn't fit our needs -

  1. I'm not a fan of throwing exceptions, I prefer handling errors gracefully.
  2. We aren't working with the OCPI HTTP API, hence such validation that throws error back to the client isn't useful for us.
    Maybe I've explained that to you in the past - we are developing an integration service between a OCPI service to a OCPP service, and we are working with Message Broker mechanism for that, so throwing validation exceptions doesn't fit that mechanism of ours.

Lior

@LiorBenAri LiorBenAri requested a review from YuriyDurov February 8, 2024 14:19
@YuriyDurov
Copy link
Member

@LiorBenAri Sure, it is not required to add validation logic right now, it can always be added later. Thank you for adding the contract models. Just make sure you have some other way in your project to check if those fields are present in the incoming data.

Regarding OcpiValidate method and OCPI HTTP API: You are not forced to use OcpiController class or it's OcpiValidate method. The package was built to be modular and you can use some parts without using others. You can use validators without using controllers - just resolve the validator directly from your DI container, and call it's Validate method. This method will return a validation result and not throw exceptions. You can then decide what to do with this result yourself. The validation result is built by the FluentValidation package.

Thank you for pointing out some other validators are missing, too. I will try to add those later if I can. The package is far from perfect and does require some work in order for it to become feature-complete.

This looks great. I am merging this.

@YuriyDurov YuriyDurov merged commit 94301a3 into BitzArt:main Feb 8, 2024
@LiorBenAri
Copy link
Contributor Author

Thank you Yuri.
I like you to know that the package is great and it is super helpful for us for our OCPI development!

@LiorBenAri
Copy link
Contributor Author

@YuriyDurov
You gonna publish a new version of the package with this last changes?

@YuriyDurov
Copy link
Member

YuriyDurov commented Feb 8, 2024

@LiorBenAri Yes it's published, the new package version is 0.16.2 and it contains these changes.

And thank you for the kind words :)

@LiorBenAri
Copy link
Contributor Author

Thank you :)

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.

2 participants