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

Fix human-readable name for ImageMetadata #1558

Merged
merged 6 commits into from
Aug 10, 2023

Conversation

praseodym
Copy link
Contributor

@praseodym praseodym commented Aug 4, 2023

Changes

The Webpage Capture boefje creates images from HostnameHTTPURL OOIs,
which means that after the normalizer creates an ImageMetadata OOI
its reference points to a HostnameHTTPURL OOI instead of an HTTPResource
OOI. #1560 was created to fix this, but for
now we can fix the human-readable name by trying to parse the reference
as a HTTPResource first and HostnameHTTPURL if this fails.

This fixes the "IndexError at /en/mispoes/objects/, pop from empty list"
error.

Issue link

Fixes #1539

Proof

Please add some proof of your working change here, unless this is not required (e.g. this PR is trivial).


Code Checklist

  • All the commits in this PR are properly PGP-signed and verified;
  • This PR only contains functionality relevant to the issue; tickets have been created for newly discovered issues.
  • I have written unit tests for the changes or fixes I made.
  • For any non-trivial functionality, I have added integration and/or end-to-end tests.
  • I have performed a self-review of my code and refactored it to the best of my abilities.

Communication

  • I have informed others of any required .env changes files if required and changed the .env-dist accordingly.
  • I have made corresponding changes to the documentation, if necessary.

Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.


Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

@praseodym praseodym requested a review from a team as a code owner August 4, 2023 14:34
@underdarknl
Copy link
Contributor

As discussed, I don't think this is the solution, Not accepting HostnameHttpUrls and IPHttpUrls seems too strict too. The model might need to be more flexible, and the normalizer more flexible about the input_ooi type

@praseodym praseodym marked this pull request as draft August 7, 2023 08:05
@praseodym praseodym force-pushed the fix/imagemetadata-human-readable branch from dfadacf to f48753b Compare August 7, 2023 09:40
@praseodym praseodym marked this pull request as ready for review August 7, 2023 09:41
The Webpage Capture boefje creates images from HostnameHTTPURL OOIs,
which means that after the normalizer creates an ImageMetadata OOI
its reference points to a HostnameHTTPURL OOI instead of an HTTPResource
OOI. #1560 was created to fix this, but for
now we can fix the human-readable name by trying to the reference as a
HTTPResource first and HostnameHTTPURL if this fails.

This fixes the "IndexError at /en/mispoes/objects/, pop from empty list"
error.
@praseodym praseodym force-pushed the fix/imagemetadata-human-readable branch from f48753b to 06dc222 Compare August 7, 2023 09:41
@praseodym
Copy link
Contributor Author

Changed the PR: first try to parse the reference
as a HTTPResource and HostnameHTTPURL if this fails.

@dekkers
Copy link
Contributor

dekkers commented Aug 7, 2023

In the model definition the reference points to only HTTPResource. That we can save objects that reference other object types is something we should fix and I don't think we should add support in the format_reference_human_readable for buggy behaviour. If ImageMetadata should be able to reference both objects we should update the model definition.

@praseodym
Copy link
Contributor Author

#1559 was created to fix the Webpage Capture boefje, #1560 was created to fix the normaliser that creates the ImageMetadata OOI.

As far as I know there is currently no way to have a ReferenceField with multiple possible types. If we were to build that, we'd have to at least add the referenced OOI's type name to the ImageMetadata OOI's natural key. This would break compatibility with existing data.

The only two ways we currently have to discern ImageMetadata OOIs referencing HostnameHTTPURL OOIs instead of an HTTPResource OOI is to count the number of | symbols in the natural key, or to try to tokenise it as one type and fall back to the other type if that fails. This PR implements the second option.

@Darwinkel
Copy link
Contributor

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset.
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • I confirmed that the PR's advertised feature or hotfix works as intended.

What works:

  • No apparent regressions
  • IndexError is gone

@dekkers dekkers merged commit 916f453 into main Aug 10, 2023
64 checks passed
@dekkers dekkers deleted the fix/imagemetadata-human-readable branch August 10, 2023 09:42
jpbruinsslot pushed a commit that referenced this pull request Aug 14, 2023
Co-authored-by: Patrick <[email protected]>
Co-authored-by: Jeroen Dekkers <[email protected]>
jpbruinsslot added a commit that referenced this pull request Aug 14, 2023
* main:
  Add sectxt dependency (#1610)
  Refactor environment settings, names, and documentation (#1517)
  Add pipeline to check if there are new translation strings (#1606)
  Translations update from Hosted Weblate (#1604)
  Update scheduler documentation (#1476)
  Add community install/update scripts (#1309)
  Bump actions/checkout from 1 to 3 (#1598)
  Run docker-compose pull in make pull (#1585)
  Configure github actions in dependabot (#1594)
  fix many ports open normalizer (#1592)
  Fix human-readable name for ImageMetadata (#1558)
  Upgrade FastAPI (#1576)
  OOI Detail page: Remember page position after clicking the "show inheritance" link (#1590)
  Fix `rstcheck` hook (#1584)
jpbruinsslot added a commit that referenced this pull request Aug 15, 2023
* main:
  Fix env setting issues found in test debian workflow (#1608)
  Add sectxt dependency (#1610)
  Refactor environment settings, names, and documentation (#1517)
  Add pipeline to check if there are new translation strings (#1606)
  Translations update from Hosted Weblate (#1604)
  Update scheduler documentation (#1476)
  Add community install/update scripts (#1309)
  Bump actions/checkout from 1 to 3 (#1598)
  Run docker-compose pull in make pull (#1585)
  Configure github actions in dependabot (#1594)
  fix many ports open normalizer (#1592)
  Fix human-readable name for ImageMetadata (#1558)
  Upgrade FastAPI (#1576)
  OOI Detail page: Remember page position after clicking the "show inheritance" link (#1590)
  Fix `rstcheck` hook (#1584)
  Manage boefjes requirements with Poetry (#1572)
praseodym added a commit that referenced this pull request Aug 21, 2023
Co-authored-by: Patrick <[email protected]>
Co-authored-by: Jeroen Dekkers <[email protected]>
dekkers added a commit that referenced this pull request Aug 21, 2023
Co-authored-by: Patrick <[email protected]>
Co-authored-by: Jeroen Dekkers <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IndexError at /en/mispoes/objects/, pop from empty list
4 participants