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

Patch existing entities in POST /bundle/import #1383

Merged
merged 148 commits into from
Oct 13, 2023
Merged

Conversation

frenchy64
Copy link
Contributor

@frenchy64 frenchy64 commented Sep 11, 2023

Epic https://github.com/advthreat/iroh/issues/7341
Related https://github.com/advthreat/iroh/issues/8207

Current behavior of bundle import on existing entities is to ignore them. This PR changes behavior to patch in this case, and loosens the schema for bundle import so then partial entities are allowed if patching.

§ QA

This PR adds the ability for POST /bundle/import to patch entities. The following steps test if this procedure works.

  1. Create an existing entity (for example an Indicator) and note its id.
  2. Patch this entity using POST /bundle/import. For example, patching the :producer field of the indicator:
POST /bundle/import?patch-existing=true
{:indicators [{:id "<id-from-step-1>" :producer "PATCHED"}]}
  1. Verify that the result looks something like this (the "updated" part is the most important part):
{:results [{:result "updated" :id "<id-from-step-1>"}]}
  1. Look up the indicator via GET /ctia/indicator to verify that the producer field was updated.

§ Release Notes

intern: Patch existing entities in POST /bundle/import

§ Squashed Commits

@frenchy64 frenchy64 requested a review from ereteog October 9, 2023 15:47
Copy link
Contributor

@ereteog ereteog left a comment

Choose a reason for hiding this comment

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

LGTM

That's a very sensitive PR, it would be nice to extend the QA section with the merge strategy for asset properties and dedicated tests to actively test the scenario behind this PR with score update.

resources/ctia/public/doc/bulk-bundle.org Outdated Show resolved Hide resolved
src/ctia/flows/crud.clj Outdated Show resolved Hide resolved
src/ctia/http/routes/crud.clj Outdated Show resolved Hide resolved
src/ctia/schemas/core.clj Outdated Show resolved Hide resolved
src/ctia/schemas/core.clj Outdated Show resolved Hide resolved
"foouser"
"foogroup"
"user")
(let [create-response (th/POST app
Copy link
Contributor

Choose a reason for hiding this comment

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

🟠 why did you need to use the http layer (+ authorization) instead of the existing bundle/import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't figure out how to make the test run on my machine.

test/ctia/bundle/routes_test.clj Outdated Show resolved Hide resolved
resources/ctia/public/doc/bulk-bundle.org Show resolved Hide resolved
resources/ctia/public/doc/bulk-bundle.org Show resolved Hide resolved
Comment on lines +125 to +126
(if (empty? asset_refs)
{}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that's now it's already included in the first test of the loop, right?:

           entities (loop [asset_refs asset_refs
                          entities {}]
                     (if (empty? asset_refs)
                       entities

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just left it in to skip a useless log.

@frenchy64 frenchy64 merged commit f077c14 into master Oct 13, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants