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.

Sign up for GitHub

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

Implements PortfolioProperties across the stack #139

Merged
merged 3 commits into from
Jan 16, 2024
Merged

Implements PortfolioProperties across the stack #139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1c26c6
Implements PortfolioProperties across the stack
gbdubs Jan 13, 2024
c7fb811
Fixes Non-Cloned Pointers, a great catch by the tests!
gbdubs Jan 13, 2024
fee2634
Merge branch 'main' in, and do review comments (oops)
gbdubs Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions azure/azevents/azevents.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,22 +299,21 @@ func (s *Server) handleParsedPortfolio(id string, resp *task.ParsePortfolioRespo
if len(incompleteUploads) == 0 {
return fmt.Errorf("no incomplete uploads found for ids: %v", resp.Request.IncompleteUploadIDs)
}
var holdingsDate *pacta.HoldingsDate
var properties *pacta.PortfolioProperties
var ownerID pacta.OwnerID
for _, iu := range incompleteUploads {
if ownerID == "" {
ownerID = iu.Owner.ID
} else if ownerID != iu.Owner.ID {
return fmt.Errorf("multiple owners found for incomplete uploads: %+v", incompleteUploads)
}
if iu.HoldingsDate == nil {
return fmt.Errorf("incomplete upload %s had no holdings date", iu.ID)
}
if holdingsDate == nil {
holdingsDate = iu.HoldingsDate
} else if iu.HoldingsDate != nil && *holdingsDate != *iu.HoldingsDate {
// Question for Grady: can iu.HoldingsDate ever be nil?
return fmt.Errorf("multiple holdings dates found for incomplete uploads: %+v", incompleteUploads)

if properties == nil {
properties = &iu.Properties
} else if *properties != iu.Properties {
// TODO(#75) We currently don't support merging portfolios with different properties.
// but we could change that if we get better input output correlation information.
return fmt.Errorf("multiple properties found for incomplete uploads: %+v", incompleteUploads)
}
if iu.RanAt.After(ranAt) {
ranAt = iu.RanAt
Expand All @@ -330,7 +329,7 @@ func (s *Server) handleParsedPortfolio(id string, resp *task.ParsePortfolioRespo
Name: output.Blob.FileName,
NumberOfRows: output.LineCount,
Blob: &pacta.Blob{ID: blobID},
HoldingsDate: holdingsDate,
Properties: *properties,
})
if err != nil {
return fmt.Errorf("creating portfolio %d: %w", i, err)
Expand Down
10 changes: 10 additions & 0 deletions cmd/server/pactasrv/conv/oapi_to_pacta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,16 @@ func LanguageFromOAPI(l api.Language) (pacta.Language, error) {
return "", oapierr.BadRequest("unknown language", zap.String("language", string(l)))
}

func OptionalBoolFromOAPI(b api.OptionalBoolean) *bool {
switch b {
case api.OptionalBooleanFALSE:
return ptr(false)
case api.OptionalBooleanTRUE:
return ptr(true)
}
return nil
}

func InitiativeCreateFromOAPI(i *api.InitiativeCreate) (*pacta.Initiative, error) {
if i == nil {
return nil, oapierr.BadRequest("InitiativeCreate cannot be nil")
Expand Down
76 changes: 49 additions & 27 deletions cmd/server/pactasrv/conv/pacta_to_oapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@ func LanguageToOAPI(l pacta.Language) (api.Language, error) {
}
}

func optionalBoolToOAPI(b *bool) api.OptionalBoolean {
if b == nil {
return api.OptionalBooleanUNSET
} else if *b {
return api.OptionalBooleanTRUE
} else {
return api.OptionalBooleanFALSE
}
}

func InitiativeToOAPI(i *pacta.Initiative) (*api.Initiative, error) {
if i == nil {
return nil, oapierr.Internal("initiativeToOAPI: can't convert nil pointer")
Expand Down Expand Up @@ -190,25 +200,31 @@ func IncompleteUploadToOAPI(iu *pacta.IncompleteUpload) (*api.IncompleteUpload,
if iu == nil {
return nil, oapierr.Internal("incompleteUploadToOAPI: can't convert nil pointer")
}
hd, err := HoldingsDateToOAPI(iu.HoldingsDate)
if err != nil {
return nil, oapierr.Internal("incompleteUploadToOAPI: holdingsDateToOAPI failed", zap.Error(err))
}
fc, err := FailureCodeToOAPI(iu.FailureCode)
if err != nil {
return nil, oapierr.Internal("incompleteUploadToOAPI: failureCodeToOAPI failed", zap.Error(err))
}
var hd *api.HoldingsDate
if iu.Properties.HoldingsDate != nil {
hd, err = HoldingsDateToOAPI(iu.Properties.HoldingsDate)
if err != nil {
return nil, oapierr.Internal("incompleteUploadToOAPI: holdingsDateToOAPI failed", zap.Error(err))
}
}
return &api.IncompleteUpload{
Id: string(iu.ID),
Name: iu.Name,
Description: iu.Description,
HoldingsDate: hd,
CreatedAt: iu.CreatedAt,
RanAt: timeToNilable(iu.RanAt),
CompletedAt: timeToNilable(iu.CompletedAt),
FailureCode: fc,
FailureMessage: stringToNilable(iu.FailureMessage),
AdminDebugEnabled: iu.AdminDebugEnabled,
Id: string(iu.ID),
Name: iu.Name,
Description: iu.Description,
CreatedAt: iu.CreatedAt,
RanAt: timeToNilable(iu.RanAt),
CompletedAt: timeToNilable(iu.CompletedAt),
FailureCode: fc,
FailureMessage: stringToNilable(iu.FailureMessage),
AdminDebugEnabled: iu.AdminDebugEnabled,
PropertyHoldingsDate: hd,
PropertyESG: optionalBoolToOAPI(iu.Properties.ESG),
PropertyExternal: optionalBoolToOAPI(iu.Properties.External),
PropertyEngagementStrategy: optionalBoolToOAPI(iu.Properties.EngagementStrategy),
Comment on lines +224 to +227
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}, nil
}

Expand All @@ -220,10 +236,6 @@ func PortfolioToOAPI(p *pacta.Portfolio) (*api.Portfolio, error) {
if p == nil {
return nil, oapierr.Internal("portfolioToOAPI: can't convert nil pointer")
}
hd, err := HoldingsDateToOAPI(p.HoldingsDate)
if err != nil {
return nil, oapierr.Internal("portfolioToOAPI: holdingsDateToOAPI failed", zap.Error(err))
}
portfolioGroupMemberships := []api.PortfolioGroupMembershipPortfolioGroup{}
for _, m := range p.PortfolioGroupMemberships {
pg, err := PortfolioGroupToOAPI(m.PortfolioGroup)
Expand All @@ -239,16 +251,26 @@ func PortfolioToOAPI(p *pacta.Portfolio) (*api.Portfolio, error) {
if err != nil {
return nil, oapierr.Internal("initiativeToOAPI: portfolioInitiativeMembershipToOAPIInitiative failed", zap.Error(err))
}
var hd *api.HoldingsDate
if p.Properties.HoldingsDate != nil {
hd, err = HoldingsDateToOAPI(p.Properties.HoldingsDate)
if err != nil {
return nil, oapierr.Internal("portfolioToOAPI: holdingsDateToOAPI failed", zap.Error(err))
}
}
return &api.Portfolio{
Id: string(p.ID),
Name: p.Name,
Description: p.Description,
HoldingsDate: hd,
CreatedAt: p.CreatedAt,
NumberOfRows: p.NumberOfRows,
AdminDebugEnabled: p.AdminDebugEnabled,
Groups: &portfolioGroupMemberships,
Initiatives: &pims,
Id: string(p.ID),
Name: p.Name,
Description: p.Description,
CreatedAt: p.CreatedAt,
NumberOfRows: p.NumberOfRows,
AdminDebugEnabled: p.AdminDebugEnabled,
Groups: &portfolioGroupMemberships,
Initiatives: &pims,
PropertyHoldingsDate: hd,
PropertyESG: optionalBoolToOAPI(p.Properties.ESG),
PropertyExternal: optionalBoolToOAPI(p.Properties.External),
PropertyEngagementStrategy: optionalBoolToOAPI(p.Properties.EngagementStrategy),
}, nil
}

Expand Down
9 changes: 9 additions & 0 deletions cmd/server/pactasrv/portfolio.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,15 @@ func (s *Server) UpdatePortfolio(ctx context.Context, request api.UpdatePortfoli
if request.Body.Description != nil {
mutations = append(mutations, db.SetPortfolioDescription(*request.Body.Description))
}
if request.Body.PropertyESG != nil {
mutations = append(mutations, db.SetPortfolioPropertyESG(conv.OptionalBoolFromOAPI(*request.Body.PropertyESG)))
}
if request.Body.PropertyExternal != nil {
mutations = append(mutations, db.SetPortfolioPropertyExternal(conv.OptionalBoolFromOAPI(*request.Body.PropertyExternal)))
}
if request.Body.PropertyEngagementStrategy != nil {
mutations = append(mutations, db.SetPortfolioPropertyEngagementStrategy(conv.OptionalBoolFromOAPI(*request.Body.PropertyEngagementStrategy)))
}
if request.Body.AdminDebugEnabled != nil {
if *request.Body.AdminDebugEnabled {
if err := s.portfolioDoAuthzAndAuditLog(ctx, id, pacta.AuditLogAction_EnableAdminDebug); err != nil {
Expand Down
21 changes: 14 additions & 7 deletions cmd/server/pactasrv/upload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,17 @@ func (s *Server) StartPortfolioUpload(ctx context.Context, request api.StartPort
return nil, err
}
owner := &pacta.Owner{ID: actorInfo.OwnerID}
holdingsDate, err := conv.HoldingsDateFromOAPI(&request.Body.HoldingsDate)
if err != nil {
return nil, err
properties := pacta.PortfolioProperties{}
if request.Body.PropertyHoldingsDate != nil {
properties.HoldingsDate, err = conv.HoldingsDateFromOAPI(request.Body.PropertyHoldingsDate)
if err != nil {
return nil, err
}
}
properties.ESG = conv.OptionalBoolFromOAPI(request.Body.PropertyESG)
properties.External = conv.OptionalBoolFromOAPI(request.Body.PropertyExternal)
properties.EngagementStrategy = conv.OptionalBoolFromOAPI(request.Body.PropertyEngagementStrategy)

n := len(request.Body.Items)
if n > 25 {
// TODO(#71) Implement basic limits
Expand Down Expand Up @@ -76,10 +83,10 @@ func (s *Server) StartPortfolioUpload(ctx context.Context, request api.StartPort
}
blob.ID = blobID
iuid, err := s.DB.CreateIncompleteUpload(tx, &pacta.IncompleteUpload{
Blob: blob,
Name: blob.FileName,
HoldingsDate: holdingsDate,
Owner: owner,
Blob: blob,
Name: blob.FileName,
Properties: properties,
Owner: owner,
})
if err != nil {
return fmt.Errorf("creating incomplete upload %d: %w", i, err)
Expand Down
50 changes: 46 additions & 4 deletions db/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,30 @@ func SetPortfolioDescription(value string) UpdatePortfolioFn {
}
}

func SetPortfolioHoldingsDate(value *pacta.HoldingsDate) UpdatePortfolioFn {
func SetPortfolioPropertyHoldingsDate(value *pacta.HoldingsDate) UpdatePortfolioFn {
return func(v *pacta.Portfolio) error {
v.HoldingsDate = value
v.Properties.HoldingsDate = value
return nil
}
}

func SetPortfolioPropertyESG(value *bool) UpdatePortfolioFn {
return func(v *pacta.Portfolio) error {
v.Properties.ESG = value
return nil
}
}

func SetPortfolioPropertyExternal(value *bool) UpdatePortfolioFn {
return func(v *pacta.Portfolio) error {
v.Properties.External = value
return nil
}
}

func SetPortfolioPropertyEngagementStrategy(value *bool) UpdatePortfolioFn {
return func(v *pacta.Portfolio) error {
v.Properties.EngagementStrategy = value
return nil
}
}
Expand Down Expand Up @@ -385,9 +406,30 @@ func SetIncompleteUploadFailureMessage(value string) UpdateIncompleteUploadFn {
}
}

func SetIncompleteUploadHoldingsDate(value *pacta.HoldingsDate) UpdateIncompleteUploadFn {
func SetIncompleteUploadPropertyHoldingsDate(value *pacta.HoldingsDate) UpdateIncompleteUploadFn {
return func(v *pacta.IncompleteUpload) error {
v.Properties.HoldingsDate = value
return nil
}
}

func SetIncompleteUploadPropertyESG(value *bool) UpdateIncompleteUploadFn {
return func(v *pacta.IncompleteUpload) error {
v.Properties.ESG = value
return nil
}
}

func SetIncompleteUploadPropertyExternal(value *bool) UpdateIncompleteUploadFn {
return func(v *pacta.IncompleteUpload) error {
v.Properties.External = value
return nil
}
}

func SetIncompleteUploadPropertyEngagementStrategy(value *bool) UpdateIncompleteUploadFn {
return func(v *pacta.IncompleteUpload) error {
v.HoldingsDate = value
v.Properties.EngagementStrategy = value
return nil
}
}
Expand Down
6 changes: 3 additions & 3 deletions db/sqldb/golden/human_readable_schema.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,10 @@ CREATE TABLE incomplete_upload (
description text NOT NULL,
failure_code failure_code,
failure_message text,
holdings_date timestamp with time zone,
id text NOT NULL,
name text NOT NULL,
owner_id text NOT NULL,
properties jsonb DEFAULT '{}'::jsonb NOT NULL,
ran_at timestamp with time zone);
ALTER TABLE ONLY incomplete_upload ADD CONSTRAINT incomplete_upload_pkey PRIMARY KEY (id);
CREATE INDEX incomplete_upload_by_blob_id ON incomplete_upload USING btree (blob_id);
Expand Down Expand Up @@ -226,11 +226,11 @@ CREATE TABLE portfolio (
blob_id text NOT NULL,
created_at timestamp with time zone DEFAULT now() NOT NULL,
description text NOT NULL,
holdings_date timestamp with time zone,
id text NOT NULL,
name text NOT NULL,
number_of_rows integer,
owner_id text NOT NULL);
owner_id text NOT NULL,
properties jsonb DEFAULT '{}'::jsonb NOT NULL);
ALTER TABLE ONLY portfolio ADD CONSTRAINT portfolio_pkey PRIMARY KEY (id);
CREATE INDEX portfolio_by_blob_id ON portfolio USING btree (blob_id);
ALTER TABLE ONLY portfolio ADD CONSTRAINT portfolio_blob_id_fkey FOREIGN KEY (blob_id) REFERENCES blob(id) ON DELETE RESTRICT;
Expand Down
8 changes: 4 additions & 4 deletions db/sqldb/golden/schema_dump.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,12 +250,12 @@ CREATE TABLE public.incomplete_upload (
blob_id text,
name text NOT NULL,
description text NOT NULL,
holdings_date timestamp with time zone,
created_at timestamp with time zone DEFAULT now() NOT NULL,
ran_at timestamp with time zone,
completed_at timestamp with time zone,
failure_code public.failure_code,
failure_message text
failure_message text,
properties jsonb DEFAULT '{}'::jsonb NOT NULL
);


Expand Down Expand Up @@ -387,10 +387,10 @@ CREATE TABLE public.portfolio (
name text NOT NULL,
description text NOT NULL,
created_at timestamp with time zone DEFAULT now() NOT NULL,
holdings_date timestamp with time zone,
blob_id text NOT NULL,
admin_debug_enabled boolean NOT NULL,
number_of_rows integer
number_of_rows integer,
properties jsonb DEFAULT '{}'::jsonb NOT NULL
);


Expand Down
Loading