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

Validate and set ps_component from purl #832

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Validate and set ps_component from purl #832

merged 1 commit into from
Dec 2, 2024

Conversation

jobselko
Copy link
Contributor

@jobselko jobselko commented Nov 20, 2024 •
edited
Loading

This PR adds validation between ps_component and purl to check they match each other and also enables the automatic setting of ps_component if a request contains only purl.

Closes OSIDB-3410

@jobselko jobselko self-assigned this Nov 20, 2024
@jobselko jobselko marked this pull request as ready for review November 20, 2024 12:56
@jobselko jobselko requested a review from a team November 20, 2024 12:56
JakubFrejlach
JakubFrejlach requested changes Nov 20, 2024
View reviewed changes
osidb/api_views.py Outdated Show resolved Hide resolved
@jobselko jobselko marked this pull request as draft November 20, 2024 14:58
@jobselko jobselko force-pushed the purl_validation branch 7 times, most recently from 3c4945a to 7b8e4ff Compare November 21, 2024 17:50
@jobselko jobselko marked this pull request as ready for review November 21, 2024 18:03
dmonzonis
dmonzonis approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@dmonzonis dmonzonis left a comment

Choose a reason for hiding this comment

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

LGTM

osoukup
osoukup requested changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

This looks almost OK but I would like the alert messages to be improved. Also it is not clear to me what happens when neither the component (which is no optional) nor PURL is provided. Will uniqueness constrain prevent this to be stored in the database? Will the error be human-readable? I would like to see a test for this case to prevent any potential future regression.

osidb/models/affect.py Outdated Show resolved Hide resolved
osidb/models/affect.py Outdated Show resolved Hide resolved
osidb/models/affect.py Outdated Show resolved Hide resolved
osidb/serializer.py Outdated Show resolved Hide resolved
@jobselko jobselko force-pushed the purl_validation branch 4 times, most recently from c061ad1 to 1d1b316 Compare November 25, 2024 12:39
@jobselko jobselko requested a review from osoukup November 25, 2024 12:55
osoukup
osoukup requested changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

A bit better but I still do not think that the validations are correct or on the correct place

osidb/models/affect.py Outdated Show resolved Hide resolved
osidb/models/affect.py Outdated Show resolved Hide resolved
osidb/models/affect.py Outdated Show resolved Hide resolved
@jobselko jobselko force-pushed the purl_validation branch 3 times, most recently from 67d1cc6 to 2909ad9 Compare November 26, 2024 15:49
osoukup
osoukup requested changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

Thank you for moving the validation logic to the validations. The non-caught exception is however still there. This seems to be the very last issue I have here

osidb/models/affect.py Outdated Show resolved Hide resolved
@jobselko jobselko requested a review from osoukup November 27, 2024 12:52
@jobselko jobselko marked this pull request as draft November 27, 2024 15:56
osoukup
osoukup reviewed Nov 28, 2024
View reviewed changes
osidb/mixins.py Outdated Show resolved Hide resolved
@jobselko jobselko force-pushed the purl_validation branch 2 times, most recently from 0f7a91e to 01693bf Compare December 2, 2024 12:36
@jobselko jobselko marked this pull request as ready for review December 2, 2024 12:41
osoukup
osoukup approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

LGTM

JakubFrejlach
JakubFrejlach approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@JakubFrejlach JakubFrejlach left a comment

Choose a reason for hiding this comment

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

LGTM! Great job.

@jobselko jobselko added this pull request to the merge queue Dec 2, 2024
Merged via the queue into master with commit 3e04fcd Dec 2, 2024
12 checks passed
@jobselko jobselko deleted the purl_validation branch December 2, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@osoukup osoukup osoukup approved these changes

@JakubFrejlach JakubFrejlach JakubFrejlach approved these changes

@dmonzonis dmonzonis dmonzonis approved these changes

Assignees

@jobselko jobselko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jobselko @dmonzonis @JakubFrejlach @osoukup