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

Fix orderModelv2 for nullable profile column #7907

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix orderModelv2 for nullable profile column #7907

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
13035df
Add tests for order model migration+rollback
aarongable Dec 19, 2024
13eaba0
Fix orderModelv2 for nullable profile column
aarongable Dec 19, 2024
d7597c4
Undo typo in sa.go
aarongable Dec 19, 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
10 changes: 7 additions & 3 deletions sa/model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ type orderModelv2 struct {
Error []byte
CertificateSerial string
BeganProcessing bool
CertificateProfileName string
CertificateProfileName *string
Copy link
Member

@pgporada pgporada Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There should be an orderModelv2 struct comment to match similar wording on the crlShardModel struct.

// CertificateProfileName is a pointer because it is a NULL-able column.

}

type orderToAuthzModel struct {
Expand Down Expand Up @@ -464,7 +464,7 @@ func orderToModelv2(order *corepb.Order) (*orderModelv2, error) {
Created: order.Created.AsTime(),
BeganProcessing: order.BeganProcessing,
CertificateSerial: order.CertificateSerial,
CertificateProfileName: order.CertificateProfileName,
CertificateProfileName: &order.CertificateProfileName,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's surprising that the resulting orderModelv2 contains a pointer to the original corepb.Order's CertificateProfileName, so changing one will change the other.

It would be better to create a new string with a copy of the input, for avoidance of surprising behavior.

Copy link
Member

Choose a reason for hiding this comment

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

@jsha That would look like this?

func orderToModelv2(order *corepb.Order) (*orderModelv2, error) {
	profile := &order.CertificateProfileName
	om := &orderModelv2{
		...
		CertificateProfileName: profile,
	}
...

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, that still results in a pointer that points to the original code. This is what I'm looking for:

profile := *order.CertificateProfileName
...
    CertificateProfileName: &profile

}

if order.Error != nil {
Expand All @@ -481,14 +481,18 @@ func orderToModelv2(order *corepb.Order) (*orderModelv2, error) {
}

func modelToOrderv2(om *orderModelv2) (*corepb.Order, error) {
profile := ""
if om.CertificateProfileName != nil {
profile = *om.CertificateProfileName
}
order := &corepb.Order{
Id: om.ID,
RegistrationID: om.RegistrationID,
Expires: timestamppb.New(om.Expires),
Created: timestamppb.New(om.Created),
CertificateSerial: om.CertificateSerial,
BeganProcessing: om.BeganProcessing,
CertificateProfileName: om.CertificateProfileName,
CertificateProfileName: profile,
}
if len(om.Error) > 0 {
var problem corepb.ProblemDetails
Expand Down
2 changes: 1 addition & 1 deletion sa/sa.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
RegistrationID: req.NewOrder.RegistrationID,
Expires: req.NewOrder.Expires.AsTime(),
Created: created,
CertificateProfileName: req.NewOrder.CertificateProfileName,
CertificateProfileName: &req.NewOrder.CertificateProfileName,
}
err = tx.Insert(ctx, &omv2)
orderID = omv2.ID
Expand Down
96 changes: 80 additions & 16 deletions sa/sa_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1354,29 +1354,17 @@ func TestOrderWithOrderModelv1(t *testing.T) {
}

func TestOrderWithOrderModelv2(t *testing.T) {
// This test requires the config-next db schema to run.
if !strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") {
t.Skip()
}

// The feature must be set before the SA is constructed because of a
// conditional on this feature in //sa/database.go.
sa, fc, cleanup := initSA(t)
defer cleanup()

features.Set(features.Config{MultipleCertificateProfiles: true})
defer features.Reset()

fc := clock.NewFake()
fc.Set(time.Date(2015, 3, 4, 5, 0, 0, 0, time.UTC))

dbMap, err := DBMapForTest(vars.DBConnSA)
test.AssertNotError(t, err, "Couldn't create dbMap")

saro, err := NewSQLStorageAuthorityRO(dbMap, nil, metrics.NoopRegisterer, 1, 0, fc, log)
test.AssertNotError(t, err, "Couldn't create SARO")

sa, err := NewSQLStorageAuthorityWrapping(saro, dbMap, metrics.NoopRegisterer)
test.AssertNotError(t, err, "Couldn't create SA")
defer test.ResetBoulderTestDatabase(t)

// Create a test registration to reference
reg := createWorkingRegistration(t, sa)
authzExpires := fc.Now().Add(time.Hour)
authzID := createPendingAuthorization(t, sa, "example.com", authzExpires)
Expand Down Expand Up @@ -1482,6 +1470,82 @@ func TestOrderWithOrderModelv2(t *testing.T) {
test.AssertDeepEquals(t, storedOrderNoName, expectedOrderNoName)
}

func TestOrderModelMigration(t *testing.T) {
// This test requires the config-next db schema to run.
if !strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") {
t.Skip()
}

sa, fc, cleanup := initSA(t)
defer cleanup()

reg := createWorkingRegistration(t, sa)

// Create an order using the v1 model
authzID := createPendingAuthorization(t, sa, "example.com", fc.Now().Add(time.Hour))
order, err := sa.NewOrderAndAuthzs(context.Background(), &sapb.NewOrderAndAuthzsRequest{
NewOrder: &sapb.NewOrderRequest{
RegistrationID: reg.Id,
Expires: timestamppb.New(fc.Now().Add(2 * time.Hour)),
DnsNames: []string{"example.com"},
V2Authorizations: []int64{authzID},
},
})
if err != nil {
t.Fatalf("failed to insert order using orderModelv1: %s", err)
}

// Retrieve that order using the v2 model
features.Set(features.Config{MultipleCertificateProfiles: true})
defer features.Reset()
storedOrder, err := sa.GetOrder(context.Background(), &sapb.OrderRequest{Id: order.Id})
if err != nil {
t.Fatalf("failed to retrieve order using orderModelv2: %s", err)
}
if storedOrder.CertificateProfileName != "" {
t.Errorf("order inserted with v1 schema should have empty profilename, instead got %q", storedOrder.CertificateProfileName)
}
}

func TestOrderModelMigrationRollback(t *testing.T) {
// This test requires the config-next db schema to run.
if !strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") {
t.Skip()
}

sa, fc, cleanup := initSA(t)
defer cleanup()

reg := createWorkingRegistration(t, sa)

// Create an order using the v2 model
features.Set(features.Config{MultipleCertificateProfiles: true})
defer features.Reset()
authzID := createPendingAuthorization(t, sa, "example.com", fc.Now().Add(time.Hour))
order, err := sa.NewOrderAndAuthzs(context.Background(), &sapb.NewOrderAndAuthzsRequest{
NewOrder: &sapb.NewOrderRequest{
RegistrationID: reg.Id,
Expires: timestamppb.New(fc.Now().Add(2 * time.Hour)),
DnsNames: []string{"example.com"},
V2Authorizations: []int64{authzID},
CertificateProfileName: "asdf",
},
})
if err != nil {
t.Fatalf("failed to insert order using orderModelv2: %s", err)
}

// Retrieve that order using the v1 model
features.Reset()
storedOrder, err := sa.GetOrder(context.Background(), &sapb.OrderRequest{Id: order.Id})
if err != nil {
t.Fatalf("failed to retrieve order using orderModelv1: %s", err)
}
if storedOrder.CertificateProfileName != "" {
t.Errorf("order retrieved with v1 schema should have empty profilename, instead got %q", storedOrder.CertificateProfileName)
}
}

// TestGetAuthorization2NoRows ensures that the GetAuthorization2 function returns
// the correct error when there are no results for the provided ID.
func TestGetAuthorization2NoRows(t *testing.T) {
Expand Down
Loading