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.

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

feat: allow partially failing batch inserts #4083

Merged
merged 27 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
4808d9c
feat: partial failures for batch import
hperl Sep 9, 2024
03b6534
chore: regenerate sdk
hperl Sep 9, 2024
db8c7a4
Merge branch 'master' into hperl/improve-batch-patch-identities-error…
hperl Sep 10, 2024
2b7e3e9
fix: implement Unwrap() on partial create error
hperl Sep 10, 2024
e2b7831
bump upload action
hperl Sep 10, 2024
01f199d
fix tests
hperl Sep 10, 2024
447c5d6
fix: mysql
hperl Sep 10, 2024
cec29ca
fix tests
hperl Sep 10, 2024
badcd6e
fix: uploading
hperl Sep 10, 2024
6fec60d
fix: wrapping
hperl Sep 10, 2024
04b302c
fix artifact upload names
hperl Sep 10, 2024
0ed2ed1
test: fix lookup E2E test
hperl Sep 11, 2024
03c127c
test: always use www.example.org as return_to URL in tests
hperl Sep 11, 2024
166302d
Merge branch 'master' into hperl/improve-batch-patch-identities-error…
zepatrik Sep 11, 2024
a2842a0
code review
hperl Sep 12, 2024
50b21ed
code review
hperl Sep 12, 2024
17ed6fe
fix test
hperl Sep 12, 2024
e491329
add batch persister test
hperl Sep 13, 2024
8a892c9
Merge remote-tracking branch 'origin/master' into hperl/improve-batch…
hperl Sep 13, 2024
3777ccf
chore: format
hperl Sep 13, 2024
9484feb
only use partial inserts on batch inserts
hperl Sep 16, 2024
afd8d6a
Merge remote-tracking branch 'origin/master' into hperl/improve-batch…
hperl Sep 16, 2024
b593fc6
code review
hperl Sep 17, 2024
99a010c
fix: usage of rows.Err and rows.Close
hperl Sep 17, 2024
efa05f6
code review
hperl Sep 17, 2024
910c590
Merge remote-tracking branch 'origin/master' into hperl/improve-batch…
hperl Sep 17, 2024
0fc3029
Merge branch 'master' into hperl/improve-batch-patch-identities-error…
hperl Sep 17, 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
12 changes: 6 additions & 6 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,9 @@ jobs:
REACT_UI_PATH: react-ui
CYPRESS_RECORD_KEY: ${{ secrets.CYPRESS_RECORD_KEY }}
- if: failure()
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v4
with:
name: logs
name: cypress-${{ matrix.database }}-logs
path: test/e2e/*.e2e.log

test-e2e-playwright:
Expand Down Expand Up @@ -320,14 +320,14 @@ jobs:
NODE_UI_PATH: node-ui
REACT_UI_PATH: react-ui
- if: failure()
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v4
with:
name: logs
name: playwright-${{ matrix.database }}-logs
path: test/e2e/*.e2e.log
- if: failure()
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v4
with:
name: playwright-test-results-${{ github.sha }}
name: playwright-test-results-${{ matrix.database }}-${{ github.sha }}
path: |
test/e2e/test-results/
test/e2e/playwright-report/
Expand Down
13 changes: 11 additions & 2 deletions identity/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -617,13 +617,22 @@ func (h *Handler) batchPatchIdentities(w http.ResponseWriter, r *http.Request, _
}
}

if err := h.r.IdentityManager().CreateIdentities(r.Context(), identities); err != nil {
err := h.r.IdentityManager().CreateIdentities(r.Context(), identities)
partialErr := new(CreateIdentitiesError)
if err != nil && !errors.As(err, &partialErr) {
h.r.Writer().WriteError(w, r, err)
return
}
for resIdx, identitiesIdx := range indexInIdentities {
if identitiesIdx != nil {
res.Identities[resIdx].IdentityID = &identities[*identitiesIdx].ID
ident := identities[*identitiesIdx]
// Check if the identity was created successfully.
if failed := partialErr.Find(ident); failed != nil {
hperl marked this conversation as resolved.
Show resolved Hide resolved
res.Identities[resIdx].Action = ActionError
res.Identities[resIdx].Error = failed.Error
} else {
res.Identities[resIdx].IdentityID = &ident.ID
}
}
}

Expand Down
41 changes: 28 additions & 13 deletions identity/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -774,34 +774,49 @@ func TestHandler(t *testing.T) {
}

for _, tt := range []struct {
name string
body *identity.CreateIdentityBody
expectStatus int
name string
body *identity.CreateIdentityBody
}{
{
name: "missing all fields",
body: &identity.CreateIdentityBody{},
expectStatus: http.StatusBadRequest,
name: "missing-all-fields",
body: &identity.CreateIdentityBody{},
},
{
name: "duplicate identity",
body: validCreateIdentityBody("valid-patch", 0),
expectStatus: http.StatusConflict,
name: "duplicate-identity",
body: validCreateIdentityBody("duplicate-identity", 0),
},
{
name: "invalid traits",
name: "invalid-traits",
body: &identity.CreateIdentityBody{
Traits: json.RawMessage(`"invalid traits"`),
},
expectStatus: http.StatusBadRequest,
},
} {
t.Run("invalid because "+tt.name, func(t *testing.T) {
patches := append([]*identity.BatchIdentityPatch{}, validPatches...)
validPatches := []*identity.BatchIdentityPatch{
{Create: validCreateIdentityBody(tt.name, 0)},
{Create: validCreateIdentityBody(tt.name, 1)},
{Create: validCreateIdentityBody(tt.name, 2)},
{Create: validCreateIdentityBody(tt.name, 3)},
{Create: validCreateIdentityBody(tt.name, 4)},
}

patches := make([]*identity.BatchIdentityPatch, 0, len(validPatches)+1)
patches = append(patches, validPatches[0:3]...)
patches = append(patches, &identity.BatchIdentityPatch{Create: tt.body})
patches = append(patches, validPatches[3:5]...)
for i, p := range patches {
id := uuid.NewV5(uuid.Nil, fmt.Sprintf("%s-%d", tt.name, i))
p.ID = &id
}

req := &identity.BatchPatchIdentitiesBody{Identities: patches}
send(t, adminTS, "PATCH", "/identities", tt.expectStatus, req)
body := send(t, adminTS, "PATCH", "/identities", http.StatusOK, req)
var actions []string
for _, a := range body.Get("identities.#.action").Array() {
actions = append(actions, a.String())
}
assert.Equal(t, []string{"create", "create", "create", "error", "create", "create"}, actions, body)
hperl marked this conversation as resolved.
Show resolved Hide resolved
})
}

Expand Down
21 changes: 11 additions & 10 deletions identity/identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,17 @@ import (
"sync"
"time"

"github.com/gofrs/uuid"
"github.com/pkg/errors"
"github.com/samber/lo"

"github.com/tidwall/sjson"

"github.com/tidwall/gjson"

"github.com/ory/kratos/cipher"
"github.com/tidwall/sjson"

"github.com/ory/herodot"
"github.com/ory/kratos/cipher"
"github.com/ory/kratos/driver/config"
"github.com/ory/x/pagination/keysetpagination"
"github.com/ory/x/sqlxx"

"github.com/ory/kratos/driver/config"

"github.com/gofrs/uuid"
"github.com/pkg/errors"
)

// An Identity's State
Expand Down Expand Up @@ -645,6 +640,9 @@ const (
// Create this identity.
ActionCreate BatchPatchAction = "create"

// Error indicates that the patch failed.
ActionError BatchPatchAction = "error"

// Future actions:
//
// Delete this identity.
Expand Down Expand Up @@ -677,4 +675,7 @@ type BatchIdentityPatchResponse struct {

// The ID of this patch response, if an ID was specified in the patch.
PatchID *uuid.UUID `json:"patch_id,omitempty"`

// The error message, if the action was "error".
Error *herodot.DefaultError `json:"error,omitempty"`
}
76 changes: 66 additions & 10 deletions identity/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import (
"context"
"encoding/json"
"fmt"
"reflect"
"slices"
"sort"
Expand Down Expand Up @@ -327,30 +328,85 @@
return len(e.availableCredentials) > 0 || len(e.availableOIDCProviders) > 0 || len(e.identifierHint) > 0
}

type FailedIdentity struct {
Identity *Identity
Error *herodot.DefaultError
}

type CreateIdentitiesError struct {
Failed []*FailedIdentity
hperl marked this conversation as resolved.
Show resolved Hide resolved
}

func (e *CreateIdentitiesError) Error() string {
return fmt.Sprintf("create identities error: %d identities failed", len(e.Failed))
}
func (e *CreateIdentitiesError) Unwrap() []error {
var errs []error
for _, failed := range e.Failed {
errs = append(errs, failed.Error)
}
return errs
}
func (e *CreateIdentitiesError) Contains(ident *Identity) bool {
for _, failed := range e.Failed {
if failed.Identity.ID == ident.ID {
return true
}
}
return false
}
func (e *CreateIdentitiesError) Find(ident *Identity) *FailedIdentity {
for _, failed := range e.Failed {
if failed.Identity.ID == ident.ID {
return failed
}
}
return nil
}
func (e *CreateIdentitiesError) ErrOrNil() error {
if len(e.Failed) == 0 {
return nil
}
return e
}

func (m *Manager) CreateIdentities(ctx context.Context, identities []*Identity, opts ...ManagerOption) (err error) {
ctx, span := m.r.Tracer(ctx).Tracer().Start(ctx, "identity.Manager.CreateIdentities")
defer otelx.End(span, &err)

for _, i := range identities {
if i.SchemaID == "" {
i.SchemaID = m.r.Config().DefaultIdentityTraitsSchemaID(ctx)
createIdentitiesError := &CreateIdentitiesError{}
validIdentities := make([]*Identity, 0, len(identities))
for _, ident := range identities {
if ident.SchemaID == "" {
ident.SchemaID = m.r.Config().DefaultIdentityTraitsSchemaID(ctx)
}

o := newManagerOptions(opts)
if err := m.ValidateIdentity(ctx, i, o); err != nil {
return err
if err := m.ValidateIdentity(ctx, ident, o); err != nil {
createIdentitiesError.Failed = append(createIdentitiesError.Failed, &FailedIdentity{
Identity: ident,
Error: herodot.ErrBadRequest.WithReasonf("%s", err).WithWrap(err),
})
continue
}
validIdentities = append(validIdentities, ident)
}

if err := m.r.PrivilegedIdentityPool().CreateIdentities(ctx, identities...); err != nil {
return err
if err := m.r.PrivilegedIdentityPool().CreateIdentities(ctx, validIdentities...); err != nil {
if partialErr := new(CreateIdentitiesError); errors.As(err, &partialErr) {
createIdentitiesError.Failed = append(createIdentitiesError.Failed, partialErr.Failed...)
} else {
return err

Check warning on line 399 in identity/manager.go

View check run for this annotation

Codecov / codecov/patch

identity/manager.go#L399

Added line #L399 was not covered by tests
}
}

for _, i := range identities {
trace.SpanFromContext(ctx).AddEvent(events.NewIdentityCreated(ctx, i.ID))
for _, ident := range validIdentities {
if !createIdentitiesError.Contains(ident) {
trace.SpanFromContext(ctx).AddEvent(events.NewIdentityCreated(ctx, ident.ID))
}
}

return nil
return createIdentitiesError.ErrOrNil()
}

func (m *Manager) requiresPrivilegedAccess(ctx context.Context, original, updated *Identity, o *ManagerOptions) (err error) {
Expand Down
6 changes: 5 additions & 1 deletion identity/pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,13 @@ type (
FindByCredentialsIdentifier(ctx context.Context, ct CredentialsType, match string) (*Identity, *Credentials, error)

// DeleteIdentity removes an identity by its id. Will return an error
// if identity exists, backend connectivity is broken, or trait validation fails.
// if identity does not exists, or backend connectivity is broken.
DeleteIdentity(context.Context, uuid.UUID) error

// DeleteIdentities removes identities by its id. Will return an error
// if any identity does not exists, or backend connectivity is broken.
DeleteIdentities(context.Context, []uuid.UUID) error

// UpdateVerifiableAddress updates an identity's verifiable address.
UpdateVerifiableAddress(ctx context.Context, address *VerifiableAddress) error

Expand Down
2 changes: 2 additions & 0 deletions internal/client-go/.openapi-generator/FILES
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ docs/Identity.md
docs/IdentityAPI.md
docs/IdentityCredentials.md
docs/IdentityCredentialsCode.md
docs/IdentityCredentialsCodeAddress.md
docs/IdentityCredentialsOidc.md
docs/IdentityCredentialsOidcProvider.md
docs/IdentityCredentialsPassword.md
Expand Down Expand Up @@ -165,6 +166,7 @@ model_health_status.go
model_identity.go
model_identity_credentials.go
model_identity_credentials_code.go
model_identity_credentials_code_address.go
model_identity_credentials_oidc.go
model_identity_credentials_oidc_provider.go
model_identity_credentials_password.go
Expand Down
1 change: 1 addition & 0 deletions internal/client-go/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ Class | Method | HTTP request | Description
- [Identity](docs/Identity.md)
- [IdentityCredentials](docs/IdentityCredentials.md)
- [IdentityCredentialsCode](docs/IdentityCredentialsCode.md)
- [IdentityCredentialsCodeAddress](docs/IdentityCredentialsCodeAddress.md)
- [IdentityCredentialsOidc](docs/IdentityCredentialsOidc.md)
- [IdentityCredentialsOidcProvider](docs/IdentityCredentialsOidcProvider.md)
- [IdentityCredentialsPassword](docs/IdentityCredentialsPassword.md)
Expand Down
Loading
Loading