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(ProtoBuf): Component.evidence optional #534

Conversation

jkowalleck
Copy link
Member

@jkowalleck jkowalleck commented Nov 4, 2024

fixes #422
by reverting the unreleased 19a1530 & acc5f3a
as discussed here: #422 (comment)

@jkowalleck jkowalleck requested a review from a team as a code owner November 4, 2024 15:13
@jkowalleck jkowalleck added this to the 1.6.1 milestone Nov 4, 2024
@jkowalleck
Copy link
Member Author

@nscuro @andreas-hilti ,

May I ask you for a review?

Would you consider this a breaking change? I've read https://protobuf.dev/programming-guides/dos-donts/#repeated-to-scalarin that subject matter.

Copy link
Contributor

@andreas-hilti andreas-hilti left a comment

Choose a reason for hiding this comment

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

For protobufs with a single evidence entry, it would be compatible, compare also:
https://protobuf.dev/programming-guides/proto3/#updating
However, for protobufs with multiple evidence entries, the merging of the elements will lead to a different behavior than what is specified in the comment (namely ignoring all but the first entry).
Thus, I tend to think it would be a breaking change.

@jkowalleck
Copy link
Member Author

jkowalleck commented Nov 4, 2024

Sorry for the confusion.

However, for protobufs with multiple evidence entries, the merging of the elements will lead to a different behavior than what is specified in the comment (namely ignoring all but the first entry).
Thus, I tend to think it would be a breaking change.

The current comment was a proposed fix for #422.
A fix that

  1. is not released, yet
  2. was made under the (false?) impression that repeated -> optional would be breaking.

The 1.6.0 diff would be the following: 1.6...jkowalleck:fork_CycloneDX-specification:fix/protobuuf-component-evidence-optional#diff-31a634760e9b4432c392ead00601567422a8cc12ac462dead1a7f7ab9fa90fdb

@jkowalleck
Copy link
Member Author

@@ -133,8 +133,8 @@ message Component {
repeated Component components = 21;
// Specifies optional, custom, properties
repeated Property properties = 22;
// Specifies optional license and copyright evidence. Only the first item in the optional repeated list is to be taken into account; every other item in the list is to be ignored/omitted.
Copy link
Member Author

Choose a reason for hiding this comment

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

Only the first item in the optional repeated list is to be taken into account; every other item in the list is to be ignored/omitted.

this was an early idea to solve the same issue. it was never publishe / released.
therefore, it is not binding.

@jkowalleck jkowalleck merged commit 0266339 into CycloneDX:master Nov 7, 2024
7 of 8 checks passed
@jkowalleck jkowalleck deleted the fix/protobuuf-component-evidence-optional branch November 7, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

component.evidence is repeated in proto and object in jsonschema
3 participants