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

Allow the artifactIndex property to be absent when the artifactLocation is within an artifact #363

Closed
lcartey opened this issue Apr 10, 2019 · 2 comments
Labels
2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. bug impact-non-breaking-change merged Changes merged into provisional draft. resolved-fixed

Comments

@lcartey
Copy link

lcartey commented Apr 10, 2019

Regarding the artifactIndex property of the artifactLocation object, the spec (section 3.4.5 on the eballot 3 draft) currently says:

If theRun.artifacts is absent or does not contain an element that describes the artifact specified by this artifactLocation object, then artifactIndex SHALL NOT be present.
...
Otherwise, artifactIndex SHOULD be present.

However, the artifactLocation is also used in the artifact object itself, where it would seem we should be able to omit the artifactIndex, as it is not required for lookup. In fact, the example in the artifactIndex section omits the artifactIndex in this way.

We could prohibit the use of artifactIndex in artifactLocations in artifact all together, but I think it might be useful to optionally allow it. This is because it would allow the use of the same in memory object for artifactLocations in both the artifact and places that reference the artifact.

@ghost ghost self-assigned this Apr 10, 2019
@ghost ghost added bug 2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. impact-non-breaking-change labels Apr 10, 2019
@ghost
Copy link

ghost commented Apr 10, 2019

@lukecartey Good catch. I added this sentence:

If thisObject occurs as the artifactLocation property (§3.23.2) of an artifact object in theRun.artifacts, then artifactIndex MAY be present. If present, it SHALL equal the array index within theRun.artifacts of the containing artifact object.

@ghost
Copy link

ghost commented Apr 10, 2019

@lukecartey Actually, in addition to adding that sentence, I reworked the presentation of the entire logic of this section. It is logically the same but, I hope, clearer.

@ghost ghost added merged Changes merged into provisional draft. resolved-fixed labels Apr 10, 2019
@ghost ghost closed this as completed Apr 10, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. bug impact-non-breaking-change merged Changes merged into provisional draft. resolved-fixed
Projects
None yet
Development

No branches or pull requests

1 participant