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.

Sign up for GitHub

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

Jump to bottom

Never match existing asset-* entities when patch-existing=false #1395

Merged
merged 13 commits into from
Dec 8, 2023
Merged

Never match existing asset-* entities when patch-existing=false #1395

merged 13 commits into from
Dec 8, 2023

Conversation

frenchy64
Copy link
Contributor

@frenchy64 frenchy64 commented Dec 8, 2023 •
edited
Loading

This is the first of two fixes to this area of code.

There are two modes for POST /bundle/import: patch-existing={true,false}. If true, then existing entities are supposed to be detected and patched.

There is a bug where asset_mappings and asset_properties are erroneously mapped onto existing assets and essentially get merged when they should be created fresh.

This PR fixes this behavior for patch-existing=false, since no-one is using patch-existing=true yet and everyone is using patch-existing=false.

The merging code was made on the faulty assumption that there can only be one asset_mapping / asset_properties per asset.

There is a red test that is fixed by commit Revert "Revert "green"". The previous commit fails with the wrong number of results:

FAIL in ctia.bundle.routes-test/ (bundle-asset-relationships-test) (routes_test.clj:1504)
es-store relationships are created for asset mappings/properties
...
expected: (= 4 (count (filter (comp #{"created"} :result) create-results2)))
  actual: (not (= 4 2))

FAIL in ctia.bundle.routes-test/ (bundle-asset-relationships-test) (routes_test.clj:1506)
es-store relationships are created for asset mappings/properties
...
expected: (= 1 (count (filter (comp #{"exists"} :result) create-results2)))
  actual: (not (= 1 3))

§ QA

No QA is needed.

§ Release Notes

intern: Never match existing asset-* entities when patch-existing=false

§ Squashed Commits

@frenchy64 frenchy64 changed the title Allow multiple mappings to asset_ref per entity Never match existing asset-* entities when patch-existing=false Dec 8, 2023
@frenchy64 frenchy64 marked this pull request as ready for review December 8, 2023 22:02
@frenchy64 frenchy64 self-assigned this Dec 8, 2023
marioaquino
marioaquino approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@marioaquino marioaquino left a comment

Choose a reason for hiding this comment

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

Seems reasonable. The patch-existing true case is covered by other tests.

@frenchy64 frenchy64 merged commit 2f1b685 into master Dec 8, 2023
14 checks passed
@frenchy64
Copy link
Contributor Author

@marioaquino yes, but they were created with the wrong assumptions (see PR description). I will need to review them ASAP. This case is just more urgent since no-one is patching yet.

ISPHOST pushed a commit that referenced this pull request Dec 11, 2023
@frenchy64 @ISPHOST
Never match existing asset-* entities when patch-existing=false (#1395)
4259a5c 
<!-- Specify linked issues and REMOVE THE UNUSED LINES -->

This is the first of two fixes to this area of code.

There are two modes for `POST /bundle/import`: `patch-existing={true,false}`. If true, then existing entities are supposed to be detected and patched.

There is a bug where `asset_mappings` and `asset_properties` are erroneously mapped onto existing assets and essentially get merged when they should be created fresh.

This PR fixes this behavior for `patch-existing=false`, since no-one is using `patch-existing=true` yet and everyone is using `patch-existing=false`.

The merging code was made on the faulty assumption that there can only be one asset_mapping / asset_properties per asset.

There is a red test that is fixed by commit [Revert "Revert "green""](79f4978). The previous commit fails with the wrong number of results:

```
FAIL in ctia.bundle.routes-test/ (bundle-asset-relationships-test) (routes_test.clj:1504)
es-store relationships are created for asset mappings/properties
...
expected: (= 4 (count (filter (comp #{"created"} :result) create-results2)))
  actual: (not (= 4 2))

FAIL in ctia.bundle.routes-test/ (bundle-asset-relationships-test) (routes_test.clj:1506)
es-store relationships are created for asset mappings/properties
...
expected: (= 1 (count (filter (comp #{"exists"} :result) create-results2)))
  actual: (not (= 1 3))
```

<!--

Describe your PR for reviewers.
Don't forget to set correct labels (User Facing / Beta / Feature Flag)
If there is UI change please add a screen capture.
-->

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="iroh-services-clients">[§](#iroh-services-clients)</a> IROH Services Clients
=====================================================================================

Put all informations that need to be communicated to IROH Services Clients.
Typically IROH-UI, ATS Integration, Orbital, etc...
 -->

<a name="qa">[§](#qa)</a> QA
============================

<!--
Describe the steps to test your PR.

1.
2.
3.

Or if no QA is needed keep it as is.
 -->
No QA is needed.

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="ops">[§](#ops)</a> Ops
===============================

  Specify Ops related issues and documentation
- Config change needed: threatgrid/tenzin#
- Migration needed: threatgrid/tenzin#
- How to enable/disable that feature: (ex remove service from `bootstrap.cfg`, add scope to org)
-->

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="documentation">[§](#documentation)</a> Documentation
=============================================================

  Public Facing documentation section;
- Public documentation updated needed: threatgrid/iroh-ui#
  See internal [doc file](./services/iroh-auth/doc/public-doc.org)
 -->

<a name="release-notes">[§](#release-notes)</a> Release Notes
=============================================================

<!-- REMOVE UNUSED LINES -->

```
intern: Never match existing asset-* entities when patch-existing=false
```

<a name="squashed-commits">[§](#squashed-commits)</a> Squashed Commits
======================================================================
@frenchy64 frenchy64 mentioned this pull request Dec 22, 2023
Merged
frenchy64 added a commit that referenced this pull request Jan 10, 2024
@frenchy64
Remove asset properties/mapping merging during bundle patch (#1408)
0ed97a1 
Followup to #1395

We always want to create new asset properties and mappings entities, even if we're patching an existing asset.

<!--

Describe your PR for reviewers.
Don't forget to set correct labels (User Facing / Beta / Feature Flag)
If there is UI change please add a screen capture.
-->

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="iroh-services-clients">[§](#iroh-services-clients)</a> IROH Services Clients
=====================================================================================

Put all informations that need to be communicated to IROH Services Clients.
Typically IROH-UI, ATS Integration, Orbital, etc...
 -->

<a name="qa">[§](#qa)</a> QA
============================

<!--
Describe the steps to test your PR.

1.
2.
3.

Or if no QA is needed keep it as is.
 -->
No QA is needed.

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="ops">[§](#ops)</a> Ops
===============================

  Specify Ops related issues and documentation
- Config change needed: threatgrid/tenzin#
- Migration needed: threatgrid/tenzin#
- How to enable/disable that feature: (ex remove service from `bootstrap.cfg`, add scope to org)
-->

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="documentation">[§](#documentation)</a> Documentation
=============================================================

  Public Facing documentation section;
- Public documentation updated needed: threatgrid/iroh-ui#
  See internal [doc file](./services/iroh-auth/doc/public-doc.org)
 -->

<a name="release-notes">[§](#release-notes)</a> Release Notes
=============================================================

<!-- REMOVE UNUSED LINES -->

```
intern: Remove asset properties/mapping merging
```

<a name="squashed-commits">[§](#squashed-commits)</a> Squashed Commits
======================================================================
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marioaquino marioaquino marioaquino approved these changes

@egierach egierach egierach approved these changes

@brookeswanson brookeswanson brookeswanson approved these changes

@gbuisson gbuisson Awaiting requested review from gbuisson gbuisson is a code owner

@yogsototh yogsototh Awaiting requested review from yogsototh yogsototh is a code owner

@msprunck msprunck Awaiting requested review from msprunck msprunck is a code owner

@ereteog ereteog Awaiting requested review from ereteog ereteog is a code owner

@turbodog99 turbodog99 Awaiting requested review from turbodog99 turbodog99 is a code owner

@rplevy rplevy Awaiting requested review from rplevy

@wandersoncferreira wandersoncferreira Awaiting requested review from wandersoncferreira wandersoncferreira is a code owner

@DeLaGuardo DeLaGuardo Awaiting requested review from DeLaGuardo DeLaGuardo is a code owner

@obarbeau obarbeau Awaiting requested review from obarbeau obarbeau is a code owner

@safi safi Awaiting requested review from safi safi is a code owner

@t2sw t2sw Awaiting requested review from t2sw t2sw is a code owner

Assignees

@frenchy64 frenchy64

Labels
No QA Required review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@frenchy64 @marioaquino @egierach @brookeswanson @trcollier