euniversal / umei-api-specification

UMEI API specifications
Apache License 2.0
3 stars 0 forks source link

add the concept of flexibility zone which is required for the Portuguese demo #55

Closed cdmNSIDE closed 2 years ago

cdmNSIDE commented 2 years ago
narve commented 2 years ago

Hi,

first of all: This is functionality that NODES will not support, and we don't have very strong opinions on these endpoints. In our server they will simply throw a suitable error (FlexilibityZonesNotSupported or something like that).

Further comments below:

narve commented 2 years ago

Nested/hiearchical endpoints, in this case /FlexibilityZones/portfolios: I generally don't recommend them. In real life they usually turn out to become messy, as most data models can be accessed through multiple "paths". If we were to use them, they should be more fully "pathified", e.g. /FlexibilityZones/{flexibilityzoneId}/portfolios.

So far we've used a flatter hiearchy for all other data models. I would therefore prefer something like /FlexilibityZonePartialSearchResult?flexibilityZoneId=xxx. This would make our URL structure more consistent.

narve commented 2 years ago

Partial search results: This is also a somewhat contended / problematic area. There are already cases were information will be partially hidden from the search results. We've chosen to not add separate "limited" datatypes for all these cases, as this will lead to an explosion in the number of data models. Instead, we hide fields by setting them to NULL in cases were the recipient is not allowed to see them. This already applies to Orders and to Trades.

I propose that you use the same methodology for the flexibility zones. Then you can reuse the same data model (FlexibilityZone) and endpoint (/FlexibilityZones), and you can just add in the description field that not all fields will be visible to all recipients.

Again, we are not going to implement it so it does not really matter to us, but it makes the OpenAPI spec longer and harder to read (and to implement, even if it is just to throw correct exceptions). Also it makes it inconsistent.

If you decide to keep it, I suggest a more generic term and enpoint as well - maybe just a /PortfolioList?flexibilityZoneId=xxx (possibly other query params?) returning either just a string[] of portfolio-ids, or (better) a PortfolioListSearchResult containing an Items which is a string[] of ids.

cdmNSIDE commented 2 years ago

Thanks for your comments Narve! I will check with my colleagues what are the changes we want to apply and will post the modification here

cdmNSIDE commented 2 years ago

Hello @narve , What do you think of the new version ?

narve commented 2 years ago

I am sorry my review is a bit delayed, I wanted to discuss this in the meetings but the topic never came up.

First of all: This PR also introduces the "Clearing" type, which is new to me. I would suggest adding at least a description of what a clearing is, and also consider renaming it.

Furthermore, the difference between FlexibilityZone and PartialFlexibilityZone is, as far as I can see, only that the property "flexibilityNeed" is not included?

If so, I strongly suggest merging these two objects. There are several other places / data types where not all requests will return and/or require all properties of all data. This could be due to several reasons, security restrictions being one of them.

If we were to create separate types for each of these cases that would cause an explosion in the number of schemas.

Consider e.g. the POST operation - when you create a new object, e.g. Order, you don't typically specify the "id" property. But we choose not to add a separate "CreateOrder" or "OrderCreation" model for this case.

For consistency, I recommend the same approach here.

narve commented 2 years ago

However, if you do need to have them as separate object types, I suggest using the OpenAPI operators allOf.

Using allOf, one can make one model (FlexibilityZone) "extend" another (PartialFlexibilityZone, or preferrable a more descriptive name like e.g. "PublicFlexibilityZone" - partial does not say much). This means that FlexibilityZone should contain all properties of PartialFlexibilityZone, in addition to its own properties (flexibilityNeed in this case).

This will reduce the code duplication and ensure consistency.

narve commented 2 years ago

Using this approach one can also modify the endpoints to specify what they return, using oneOf:

One can say that the /FlexibilityZones endpoint returns an array of either FlexibilityZone or PartialFlexibilityZone. This removes the need for extra endpoints.

We could use this approach for various other endpoints as well... but we don't time to make things perfect :)

In addition, the code generator tools will typically generate more code, usually quite bloated, so I don't recommend this approach in all cases.

cdmNSIDE commented 2 years ago

Hello @narve I have removed the notion of Clearing because we finally don't need it. I still need to work on your other comment ;)

cdmNSIDE commented 2 years ago

Hello @narve. The PartialFlexibilityZone has been removed. We will indeed work with the authorization stuff to handle the specific case where the FSP can not have access to the flexibility need. I did not write anything about that in the UMEI as it is too specific.

This PR is ready for a final review

cdmNSIDE commented 2 years ago

Hi Narve, This PR can be merged and then closed :)