Skip to content

Commit

Permalink
feat: don't consider soft delete for interleaved rows
Browse files Browse the repository at this point in the history
This is too hard to get right for all use cases. The simplest thing to
reason about is to only consider soft deletes for the top-level table
(because that's the one that determines pagination) and let the caller
filter out interleaved results as required.

BREAKING CHANGE: Soft-deleted interleaved rows will now always be
                 returned.
  • Loading branch information
odsod committed Aug 8, 2021
1 parent 2b03d55 commit bb64398
Show file tree
Hide file tree
Showing 5 changed files with 8 additions and 118 deletions.
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ go 1.15
require (
cloud.google.com/go v0.90.0
cloud.google.com/go/spanner v1.24.0
github.com/google/go-cmp v0.5.6
github.com/stoewer/go-strcase v1.2.0
go.einride.tech/aip v0.43.0
google.golang.org/api v0.52.0
Expand Down
9 changes: 0 additions & 9 deletions internal/codegen/databasecodegen/readtransaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -405,11 +405,6 @@ func (g ReadTransactionCodeGenerator) generateReadInterleavedRowsMethod(f *codeg
f.P("r.", childName, " = make(map[", key.Type(), "][]*", row.Type(), ")")
}
f.P("if err := t.", g.ReadMethod(child), "(ctx, query.KeySet).Do(func(row *", row.Type(), ") error {")
if g.hasSoftDelete(child) {
f.P("if row.", g.softDeleteTimestampFieldName(table), ".Valid {")
f.P("return nil")
f.P("}")
}
f.P("k := ", parentKey.Type(), "{")
for _, part := range parent.PrimaryKey {
f.P(key.FieldName(part), ": row.", key.FieldName(part), ",")
Expand Down Expand Up @@ -503,10 +498,6 @@ func (g ReadTransactionCodeGenerator) softDeleteTimestampColumnName(table *spand
return "delete_time"
}

func (g ReadTransactionCodeGenerator) softDeleteTimestampFieldName(table *spanddl.Table) spansql.ID {
return "DeleteTime"
}

func rangeInterleavedTables(table *spanddl.Table, f func(parent, child *spanddl.Table)) {
for _, interleaved := range table.InterleavedTables {
f(table, interleaved)
Expand Down
3 changes: 0 additions & 3 deletions internal/codegen/databasecodegen/testdata/6.sql.database.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions internal/examples/freightdb/database_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

110 changes: 7 additions & 103 deletions internal/examples/freightdb/database_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"testing"

"cloud.google.com/go/spanner"
"github.com/google/go-cmp/cmp/cmpopts"
"go.einride.tech/spanner-aip/internal/examples/freightdb"
"go.einride.tech/spanner-aip/spantest"
"google.golang.org/grpc/codes"
Expand Down Expand Up @@ -148,20 +149,6 @@ func TestReadTransaction(t *testing.T) {
row,
)
})

t.Run("interleaved deleted", func(t *testing.T) {
t.Parallel()
tx := client.ReadOnlyTransaction()
defer tx.Close()

row, err := freightdb.Query(tx).GetShippersRow(ctx, freightdb.GetShippersRowQuery{
Key: freightdb.ShippersKey{ShipperId: "interleavedeleted"},
Shipments: true,
LineItems: true,
})
assert.NilError(t, err)
assert.DeepEqual(t, &freightdb.ShippersRow{ShipperId: "interleavedeleted"}, row)
})
})

t.Run("NotFound", func(t *testing.T) {
Expand Down Expand Up @@ -386,43 +373,6 @@ func TestReadTransaction(t *testing.T) {
got = append(got, row)
return nil
}))
assert.DeepEqual(
t,
[]*freightdb.ShippersRow{
{
ShipperId: "allexists",
Shipments: []*freightdb.ShipmentsRow{
{
ShipperId: "allexists",
ShipmentId: "allexists",
LineItems: []*freightdb.LineItemsRow{
{ShipperId: "allexists", ShipmentId: "allexists", LineNumber: 1},
{ShipperId: "allexists", ShipmentId: "allexists", LineNumber: 2},
},
},
},
},
{ShipperId: "interleavedeleted"},
},
got,
)
})

t.Run("interleaved show deleted", func(t *testing.T) {
t.Parallel()
tx := client.ReadOnlyTransaction()
defer tx.Close()

var got []*freightdb.ShippersRow
assert.NilError(t, freightdb.Query(tx).ListShippersRows(ctx, freightdb.ListShippersRowsQuery{
Limit: 10,
ShowDeleted: true,
Shipments: true,
LineItems: true,
}).Do(func(row *freightdb.ShippersRow) error {
got = append(got, row)
return nil
}))
assert.DeepEqual(
t,
[]*freightdb.ShippersRow{
Expand All @@ -440,25 +390,21 @@ func TestReadTransaction(t *testing.T) {
},
},
{
ShipperId: "deleted",
DeleteTime: spanner.NullTime{
Valid: true,
Time: commitTimestamp,
},
ShipperId: "interleavedeleted",
Shipments: []*freightdb.ShipmentsRow{
{
ShipperId: "deleted",
ShipmentId: "deleted",
ShipperId: "interleavedeleted",
ShipmentId: "interleavedeleted",
LineItems: []*freightdb.LineItemsRow{
{ShipperId: "deleted", ShipmentId: "deleted", LineNumber: 1},
{ShipperId: "deleted", ShipmentId: "deleted", LineNumber: 2},
{ShipperId: "interleavedeleted", ShipmentId: "interleavedeleted", LineNumber: 1},
{ShipperId: "interleavedeleted", ShipmentId: "interleavedeleted", LineNumber: 2},
},
},
},
},
{ShipperId: "interleavedeleted"},
},
got,
cmpopts.IgnoreFields(freightdb.ShipmentsRow{}, "DeleteTime"),
)
})
})
Expand Down Expand Up @@ -495,48 +441,6 @@ func TestReadTransaction(t *testing.T) {
assert.DeepEqual(t, expectedIDs, gotIDs)
})

t.Run("get deleted interleaved", func(t *testing.T) {
t.Parallel()
client := fx.NewDatabaseFromDDLFiles(t, "../../../testdata/migrations/freight/*.up.sql")
const count = 10
mutations := make([]*spanner.Mutation, 0, count)
shipper := &freightdb.ShippersRow{
ShipperId: "shipper",
DeleteTime: spanner.NullTime{
Time: spanner.CommitTimestamp,
Valid: true,
},
}
mutations = append(mutations, spanner.Insert(shipper.Mutate()))
expectedShipmentIDs := make([]string, 0, count)
for i := 0; i < count; i++ {
shipment := &freightdb.ShipmentsRow{ShipperId: shipper.ShipperId, ShipmentId: strconv.Itoa(i)}
if i%2 == 1 {
shipment.DeleteTime = spanner.NullTime{
Time: spanner.CommitTimestamp,
Valid: true,
}
} else {
expectedShipmentIDs = append(expectedShipmentIDs, shipment.ShipmentId)
}
mutations = append(mutations, spanner.Insert(shipment.Mutate()))
}
_, err := client.Apply(ctx, mutations)
assert.NilError(t, err)
tx := client.ReadOnlyTransaction()
defer tx.Close()
gotShipper, err := freightdb.Query(tx).GetShippersRow(ctx, freightdb.GetShippersRowQuery{
Key: freightdb.ShippersKey{ShipperId: shipper.ShipperId},
Shipments: true,
})
assert.NilError(t, err)
gotShipmentIDs := make([]string, 0, len(gotShipper.Shipments))
for _, gotShipment := range gotShipper.Shipments {
gotShipmentIDs = append(gotShipmentIDs, gotShipment.ShipmentId)
}
assert.DeepEqual(t, expectedShipmentIDs, gotShipmentIDs)
})

t.Run("show deleted", func(t *testing.T) {
t.Parallel()
client := fx.NewDatabaseFromDDLFiles(t, "../../../testdata/migrations/freight/*.up.sql")
Expand Down

0 comments on commit bb64398

Please sign in to comment.