-
Notifications
You must be signed in to change notification settings - Fork 318
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
Use implicit field mask paths in IS #249
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,6 +83,7 @@ func (is *IdentityServer) getApplication(ctx context.Context, req *ttnpb.GetAppl | |
if err = is.RequireAuthenticated(ctx); err != nil { | ||
return nil, err | ||
} | ||
req.FieldMask.Paths = cleanFieldMaskPaths(ttnpb.ApplicationFieldPathsNested, req.FieldMask.Paths, getPaths, nil) | ||
if err = rights.RequireApplication(ctx, req.ApplicationIdentifiers, ttnpb.RIGHT_APPLICATION_INFO); err != nil { | ||
if ttnpb.HasOnlyAllowedFields(req.FieldMask.Paths, ttnpb.PublicApplicationFields...) { | ||
defer func() { app = app.PublicSafe() }() | ||
|
@@ -110,6 +111,7 @@ func (is *IdentityServer) getApplication(ctx context.Context, req *ttnpb.GetAppl | |
} | ||
|
||
func (is *IdentityServer) listApplications(ctx context.Context, req *ttnpb.ListApplicationsRequest) (apps *ttnpb.Applications, err error) { | ||
req.FieldMask.Paths = cleanFieldMaskPaths(ttnpb.ApplicationFieldPathsNested, req.FieldMask.Paths, getPaths, nil) | ||
var appRights map[string]*ttnpb.Rights | ||
if req.Collaborator == nil { | ||
callerRights, _, err := is.getRights(ctx) | ||
|
@@ -186,8 +188,14 @@ func (is *IdentityServer) updateApplication(ctx context.Context, req *ttnpb.Upda | |
if err = rights.RequireApplication(ctx, req.ApplicationIdentifiers, ttnpb.RIGHT_APPLICATION_SETTINGS_BASIC); err != nil { | ||
return nil, err | ||
} | ||
if err := validateContactInfo(req.Application.ContactInfo); err != nil { | ||
return nil, err | ||
req.FieldMask.Paths = cleanFieldMaskPaths(ttnpb.ApplicationFieldPathsNested, req.FieldMask.Paths, nil, getPaths) | ||
if len(req.FieldMask.Paths) == 0 { | ||
req.FieldMask.Paths = updatePaths | ||
} | ||
if ttnpb.HasAnyField(req.FieldMask.Paths, "contact_info") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will not evaluate to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if err := validateContactInfo(req.Application.ContactInfo); err != nil { | ||
return nil, err | ||
} | ||
} | ||
err = is.withDatabase(ctx, func(db *gorm.DB) (err error) { | ||
app, err = store.GetApplicationStore(db).UpdateApplication(ctx, &req.Application, &req.FieldMask) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
Why not just set this to
getPaths
iflen(req.FieldMask.Paths) == 0
?Basically the same question for all other invocations - I really don't get what's going on here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getPaths
contains the fields that are implicitly part of the field mask, so they are always added to the field mask in the request, if it doesn't already contain them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but there's no harm in adding a field twice - you can just append the fields - we do the same in JS/NS and AS AFAIK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to construct the slice of required fields,
Set
registry endpoint in NS constructs agets
andsets
:lorawan-stack/pkg/networkserver/grpc_deviceregistry.go
Lines 49 to 57 in 59154c3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the NS/JS. In the IS we prefer to have paths in the field mask only once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then why not
ttnpb.EnsureFieldpaths(paths []string, ensure ...string) []string
?You pass the existing fieldmask and the paths you want to add if missing.
You don't need 4 arguments for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no
ttnpb.EnsureFieldPaths
, I want to combine this with the allowed paths, I want to be able to specify paths that need to be deleted if present, and finally, I want to do it in a single pass instead of combiningEnsureFieldPaths
,EnsureNotFieldPaths
,IntersectFieldPaths
,UniqueFieldPaths
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RPC implementation simply should not delete anything passed to it. Cleaning and validation belongs to the API layer middleware. RPC should simply upsert the required fields.