Implements PortfolioProperties across the stack #139
Conversation
| PropertyHoldingsDate: hd, | ||
| PropertyESG: optionalBoolToOAPI(iu.Properties.ESG), | ||
| PropertyExternal: optionalBoolToOAPI(iu.Properties.External), | ||
| PropertyEngagementStrategy: optionalBoolToOAPI(iu.Properties.EngagementStrategy), |
There was a problem hiding this comment.
It seems like an implementation detail that we internally store these things as an unstructured 'properties' blob, I'd drop the Property prefix on all of these. A HoldingsDate is a HoldingsDate regardless of how we store it in the DB.
And same throughout the API, both user-facing and at the pacta package level, I wouldn't lump all these properties under a Properties field, they have no more or less in common with each other than any other field (e.g. Name, Description) shared between multiple types.
For convenience, you could put all the shared fields between IncompleteUpload and Porfolio in some base struct and embed that.
There was a problem hiding this comment.
The reason I wanted to include this prefix is because I think it helps provide some helpful context which I think is useful to future engineers and API users. Properties differ from other fields of objects in these three ways:
1 - Our backend's role is exclusively to store them - we don't set these values, we don't base any of our business logic off of them, etc.
2 - Different Initiatives will have policies that require them to be set - a constraint that other data objects don't have.
3 - There are expected to be a pretty large number of them, and they're expected to expand over time (Hodie estimated ~1 per initiative onboarded onto the platform).
Because of these, wrapping these values in a shared struct has clear benefits:
- Unifies storage and retrieval (1)
- Makes expressions of constraint like (2) more self contained
- Reduces change costs for (3)
What do you think about that explanation?
There was a problem hiding this comment.
SG, though I'm not convinced "we don't base any of our business logic off of them" will hold for very long. It's not just an arbitrary bag of KVs that we dump on the user, we'll be making at the very least UI display decisions based on the presence/absence/value of these things right out of the gate. And "... will have policies that require them to be set" seems like a business logic consideration as well.
I don't think it's a big deal either way, though I'd note whatever constraints you expect to enforce in the OpenAPI docs, so those get propagated to whatever auto-generated docs we eventually create
| OptionalBoolean: | ||
| type: string | ||
| enum: | ||
| - OptionalBooleanTRUE | ||
| - OptionalBooleanFALSE | ||
| - OptionalBooleanUNSET |
There was a problem hiding this comment.
nit: What's wrong with just a non-required boolean? IIRC, the codegen makes non-required things pointers, and *bool conveys the tri-state (unset, false, true) that we care about. The enum seems clunky and verbose by comparison.
There was a problem hiding this comment.
Good question, three reasons:
Within update requests today, we treat all fields as optional - if they're unset we don't change them. If a nilable boolean value is in the actual object, there are actually four signals that could be sent about that value to an update endpoint (do nothing, clear, set true, set false). The only way around that would be to ALWAYS set these values on update, which would break our otherwise consistent paradigm of updates, and would be ripe for bugs (someone could easily call the 'updatePortfolio' method with a new name, not realizing that they need to set these values or they will get reset.
OK, the argument then goes, why don't we represent it differently for objects and for updates? Updates might need that fourth state, but for the actual object representation, we surely only need the three states?
Well we've taken an approach to editor fields (using highly generic methods that leverage intricate typescript features to reduce the volume of boilerplate code), and while I think that approach is a reasonable one, one downside of it is that it couples the UpdateX object to be a subset of the X object.
While we could bend-over-backwards to make it work with this as a *bool, I don't think the additional value is worth its (many) costs. Happy to discuss if you disagree!
There was a problem hiding this comment.
The explanation of four states makes sense to me, #shipit
PortfolioPropertiesto capture metadata that might be passed into the analyzers, but isn't used within the infrastructure we're building. This is stored as a JSON blob in the DB for maximum flexibility. Moves holdings date into this, since it fits that profile.OptionalBooleanfor a few of these properties, sinceupdateoperations have to differentiate between an unset state and a cleared state.