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

Question: Should we avoid having Partial field inside Partial object #34

Open
atschabu opened this issue Dec 4, 2023 · 3 comments
Open
Labels
help wanted Extra attention is needed module: all

Comments

@atschabu
Copy link
Contributor

atschabu commented Dec 4, 2023

As far as I can see, all generated Partials, use Partials as field values. For example SessionPartial.charging_periods is of type List<ChargingPeriodPartial>?, but the expectation is that we provide a valid ChargingPeriod, not partially updating existing ChargingPeriods (as that would require a PUT Session call)

When a PATCH request contains the charging_periods field (inside a Session object), this SHALL be processed as a request to add all the ChargingPeriod objects to the existing Session object.

The issue is worse with the Location object, where we have about 10+ fields that are complex objects. Unless I interpret the protocol incorrectly, only the provided root object can be Partial, while all field values should be "regular" fully valid objects. Therefore types like GeoLocation and BusinessDetail don't even need to be annotated as @Partial, and generated Partials should not use Partial fields as field types. Which means a LocationPartial should contain a list of Evse, and only when calling PATCH Evse endpoint should we using EvsePartial, etc.

@lilgallon
Copy link
Member

Yes I think you are right, I recoded generation logic here: #31, and I can make the changes you are requesting there

@lilgallon lilgallon added enhancement New feature or request module: all labels Dec 4, 2023
@lilgallon
Copy link
Member

The spec is not super clear on that, for me it would make sense to only update latitude of a Location for instance. I will try to get more information about that. Maybe there are some exceptions, like the one you stated about ChargingPeriod.

@atschabu
Copy link
Contributor Author

atschabu commented Dec 4, 2023

it looks like "it depends" on how one interprets the protocol ... I would have assumed that a change to an evse will always use the PATCH evse endpoint, but apparently that is not common consensus given how the creator of this issue experienced a partner wanting to use the PATCH location endpoint with a wonky json: ocpi/ocpi#285

assuming you have any impact on the partners implementation of the PATCH endpoint, and can make them only send PATCH for root element, no need for nested Partials ... on the other hand, if you need to be able to consume a crazy json merge object like the one in 285, you'd need nested Partials

@lilgallon lilgallon added help wanted Extra attention is needed and removed enhancement New feature or request labels Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed module: all
Projects
None yet
Development

No branches or pull requests

2 participants