trolie / spec

Transmission Ratings and Operating Limits Information Exchange
https://trolie.energy/
Other
2 stars 2 forks source link

refactor `limit` to use permissive schema #50

Closed caindy closed 4 months ago

caindy commented 4 months ago

Closes #58 Relates to #43

Kiota is a .NET client generator, and it said there would be runtime errors without a discriminator field. That was the impetus. The straightforward way to do this was to add it to every limit value instead of at the period, resource, or proposal/snapshot level. The drawbacks are pretty obvious: The limitType is mostly redundant, and this creates the possibility of mixing units for a particular rating or any given period. The latter might be considered an advantage if anyone operated that way in practice, but I don't think that's realistic.

caindy commented 4 months ago

I have an alternate approach I'd like to consider.

As noted above this could be added at a higher level, and I think the resource-level is most aligned with the domain. The issue is that OpenAPI doesn't support generics or any other way for a containing type to be parameterized by a child type. Still, we can do this by defining the limit value schema to be a superset of all the quantities, e.g.,

type: object
properties:
  mva: ...
  mw: ...
  pf: ...
  amps: ...

Note none of these is required, so runtime validation would need to be explicitly coded by the server, as schema validation is insufficient. Another way to encode this would be to use the anyOf construct, which would allow us to maintain the explicit semantics of valid limit type variations (apparent power, actual power, current), but I'd need to make sure most client generators work well with it.

@getorymckeag lmk what you think

caindy commented 4 months ago

In addition to the above, it's possible to control to reactive power and voltage, too. I propose to add those as possible quantities as well.

catkins-miso commented 4 months ago

@getorymckeag Based on our conversation last week, I am going to implement the following, adding reactive power and voltage to the supported quantities, in addition to what is listed below.

I have an alternate approach I'd like to consider.

As noted above this could be added at a higher level, and I think the resource-level is most aligned with the domain. The issue is that OpenAPI doesn't support generics or any other way for a containing type to be parameterized by a child type. Still, we can do this by defining the limit value schema to be a superset of all the quantities, e.g.,

type: object
properties:
  mva: ...
  mw: ...
  pf: ...
  amps: ...

Note none of these is required, so runtime validation would need to be explicitly coded by the server, as schema validation is insufficient. Another way to encode this would be to use the anyOf construct, which would allow us to maintain the explicit semantics of valid limit type variations (apparent power, actual power, current), but I'd need to make sure most client generators work well with it.

@getorymckeag lmk what you think

caindy commented 4 months ago

The client code generated with this is still pretty ugly.

/// <summary>The amps property</summary>
public float? Amps { get; set; }
/// <summary>The baseKV property</summary>
public int? BaseKV { get; set; }
/// <summary>The kVMax property</summary>
public int? KVMax { get; set; }
/// <summary>The kVMin property</summary>
public int? KVMin { get; set; }
/// <summary>The mva property</summary>
public float? Mva { get; set; }
/// <summary>The mvar property</summary>
public float? Mvar { get; set; }
/// <summary>The mw property</summary>
public float? Mw { get; set; }
/// <summary>The pf property</summary>
public float? Pf { get; set; }
/// <summary>The voltagePuMax property</summary>
public float? VoltagePuMax { get; set; }
/// <summary>The voltagePuMin property</summary>
public float? VoltagePuMin { get; set; }
caindy commented 4 months ago

@getorymckeag I hate to keep vacillating on this, but I'm inclined to model the spec in the most desirable way rather than compromise the semantics with a permissive schema (current approach in this PR) or introduce limitType (previous approach in this PR).

I'd like to change the limit.yaml file to the following. In practice clients are going to know ahead of time what they're allowed to send and expect to receive, so they can customize the generated client code accordingly. I think the server implementation is a bit trickier since it really should distinguish between "client error because I don't support the submitted limit type" and "client error because the limit is malformed".

description: |

  Defines the limit. In practice, most exchanges will only support one kind of
  limit for proposals and snapshots. However, the specification supports
  defining limits on a per resource basis as well as limit types that are not
  anticipated to be used to implement Order 881. TROLIE server implementations
  must support at least one of these limit types and should return an well known
  `application/problem+json` response if they receive a proposal in an
  unsupported but valid limit type as defined here.

oneOf:
  - $ref: './limit-types/active-power.yaml'
  - $ref: './limit-types/apparent-power.yaml'
  - $ref: './limit-types/current.yaml'
  - $ref: './limit-types/reactive-power.yaml'
  - $ref: './limit-types/voltage.yaml#/overvoltage-threshold-pu'
  - $ref: './limit-types/voltage.yaml#/overvoltage-threshold'
  - $ref: './limit-types/voltage.yaml#/undervoltage-threshold-pu'
  - $ref: './limit-types/voltage.yaml#/undervoltage-threshold'
caindy commented 4 months ago

I think the server implementation is a bit trickier since it really should distinguish between "client error because I don't support the submitted limit type" and "client error because the limit is malformed".

FWIW, looks like Java17 effectively supports union types: https://openjdk.org/jeps/409