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

(PXP-9480): Support merge param for PUT /resource endpoint #143

Merged
merged 5 commits into from
Feb 15, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
4 changes: 2 additions & 2 deletions .secrets.baseline
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"files": "go.sum",
"lines": null
},
"generated_at": "2021-11-29T14:09:22Z",
"generated_at": "2022-02-11T04:54:33Z",
"plugins_used": [
{
"name": "AWSKeyDetector"
Expand Down Expand Up @@ -62,7 +62,7 @@
{
"hashed_secret": "f9fdc64928c96c7ad56bf7da557f70345d83a6ed",
"is_verified": false,
"line_number": 1643,
"line_number": 1657,
"type": "Base64 High Entropy String"
}
],
Expand Down
62 changes: 33 additions & 29 deletions arborist/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
type ResourceIn struct {
Name string `json:"name"`
Path string `json:"path"`
Description string `json:"description"`
Description *string `json:"description"`
Subresources []ResourceIn `json:"subresources"`
}

Expand Down Expand Up @@ -329,7 +329,7 @@ func (resource *ResourceIn) addPath(parent string) *ErrorResponse {
return nil
}

func (resource *ResourceIn) overwriteInDb(tx *sqlx.Tx) *ErrorResponse {
func (resource *ResourceIn) updateInDb(tx *sqlx.Tx, merge bool) *ErrorResponse {
// arborist uses `/` for path separator; ltree in postgres uses `.`
path := FormatPathForDb(resource.Path)
stmt := "INSERT INTO resource(path) VALUES ($1) ON CONFLICT DO NOTHING"
Expand All @@ -345,34 +345,38 @@ func (resource *ResourceIn) overwriteInDb(tx *sqlx.Tx) *ErrorResponse {
return newErrorResponse(msg, 409, &err)
}

// update description
stmt = "UPDATE resource SET description = $2 WHERE path = $1"
_, err = tx.Exec(stmt, path, resource.Description)
if resource.Description != nil {
// update description
stmt = "UPDATE resource SET description = $2 WHERE path = $1"
_, err = tx.Exec(stmt, path, resource.Description)
}

// delete the subresources not in the new request
if len(resource.Subresources) > 0 {
subPathsKeep := []string{}
for _, subresource := range resource.Subresources {
subresource.addPath(resource.Path)
subpath := fmt.Sprintf("'%s'", FormatPathForDb(subresource.Path))
subPathsKeep = append(subPathsKeep, subpath)
if !merge {
// delete the subresources not in the new request
if len(resource.Subresources) > 0 {
subPathsKeep := []string{}
for _, subresource := range resource.Subresources {
subresource.addPath(resource.Path)
subpath := fmt.Sprintf("'%s'", FormatPathForDb(subresource.Path))
subPathsKeep = append(subPathsKeep, subpath)
}
stmtFormat := `
DELETE FROM resource
WHERE (
path != $1
AND path ~ (CAST ((ltree2text($1) || '.*{1}') AS lquery))
AND path NOT IN (%s)
)
`
stmt = fmt.Sprintf(stmtFormat, strings.Join(subPathsKeep, ", "))
_, _ = tx.Exec(stmt, path)
} else {
stmt := `
DELETE FROM resource
WHERE path != $1 AND path ~ (CAST ((ltree2text($1) || '.*{1}') AS lquery))
`
_, _ = tx.Exec(stmt, path)
}
stmtFormat := `
DELETE FROM resource
WHERE (
path != $1
AND path ~ (CAST ((ltree2text($1) || '.*{1}') AS lquery))
AND path NOT IN (%s)
)
`
stmt = fmt.Sprintf(stmtFormat, strings.Join(subPathsKeep, ", "))
_, _ = tx.Exec(stmt, path)
} else {
stmt := `
DELETE FROM resource
WHERE path != $1 AND path ~ (CAST ((ltree2text($1) || '.*{1}') AS lquery))
`
_, _ = tx.Exec(stmt, path)
}

// TODO (rudyardrichter, 2019-04-09): optimize (could be non-recursive)
Expand All @@ -382,7 +386,7 @@ func (resource *ResourceIn) overwriteInDb(tx *sqlx.Tx) *ErrorResponse {
if errResponse != nil {
return errResponse
}
errResponse = subresource.overwriteInDb(tx)
errResponse = subresource.updateInDb(tx, merge)
if errResponse != nil {
return errResponse
}
Expand Down
7 changes: 6 additions & 1 deletion arborist/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -864,7 +864,12 @@ func (server *Server) handleResourceCreate(w http.ResponseWriter, r *http.Reques

errResponse = nil
if r.Method == "PUT" {
errResponse = transactify(server.db, resource.overwriteInDb)
_, mergeFlag := r.URL.Query()["merge"]
updateResource := func(tx *sqlx.Tx) *ErrorResponse {
resource.updateInDb(tx, mergeFlag)
return nil
}
errResponse = transactify(server.db, updateResource)
} else {
errResponse = transactify(server.db, resource.createInDb)
}
Expand Down
29 changes: 29 additions & 0 deletions arborist/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1105,6 +1105,35 @@ func TestServer(t *testing.T) {
getResourceWithPath(t, "/Godel,/completeness_theorem")
})

t.Run("Merge", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we could maybe add one more test or command in here that updates an existing subresource instead of top level. So like update the dog resource, add goldenretriever or something. What's the behavior if you do that but leave out corgi? I assume it also stays around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the expected behavior would be that corgi stays around.

createResourceBytes(t, []byte(`{
"name": "animal",
"subresources": [
{"name": "dinosaur", "subresources": [{"name": "pterodactyl"}]},
{"name": "dog", "subresources": [{"name": "corgi"}]}
]
}`))
w := httptest.NewRecorder()
body := []byte(`{
"name": "animal",
"subresources": [
{"name": "bird", "subresources": [{"name": "goose"}]},
{"name": "cat", "subresources": [{"name": "lion"}]}
]
}`)
req := newRequest("PUT", "/resource?merge", bytes.NewBuffer(body))
handler.ServeHTTP(w, req)
if w.Code != http.StatusCreated {
httpError(t, w, "couldn't update resource using PUT")
}
// verify that resources created before update were not overwritten
getResourceWithPath(t, "/animal/dinosaur/pterodactyl")
getResourceWithPath(t, "/animal/dog/corgi")
// verify that update actually created resources
getResourceWithPath(t, "/animal/bird/goose")
getResourceWithPath(t, "/animal/cat/lion")
})

t.Run("Delete", func(t *testing.T) {
w := httptest.NewRecorder()
req := newRequest("DELETE", "/resource/a", nil)
Expand Down
22 changes: 18 additions & 4 deletions docs/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -383,16 +383,14 @@ paths:
schema:
$ref: '#/components/schemas/ResourceInput'
parameters:
- in: path
- in: query
name: p
description: >-
Ordinarily arborist will return an error if parent resources of a
resource you're trying to create do not exist yet. If the `p`
parameter is included, it behaves like `mkdir -p` and creates all
parent resources as necessary.
schema:
type: integer
required: true
required: false
responses:
201:
description: JSON representation of successfully-created resource
Expand Down Expand Up @@ -421,6 +419,22 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/ResourceInput'
parameters:
- in: query
name: p
description: >-
Ordinarily arborist will return an error if parent resources of a
resource you're trying to create do not exist yet. If the `p`
parameter is included, it behaves like `mkdir -p` and creates all
parent resources as necessary.
required: false
- in: query
name: merge
description: >-
Ordinarily arborist will overwrite the resource by deleting
existing subresources. If the `merge` parameter is included,
those existing subresources are not deleted.
required: false
responses:
201:
description: JSON representation of successfully-updated resource
Expand Down