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

result.message SHALL be present constraint should be added to schema #283

Closed
michaelcfanning opened this issue Nov 15, 2018 · 8 comments
Closed
Labels
2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. bug change-draft-superfluous The change is a simple rename; no change draft needed. impact-breaks-producers triage-approved

Comments

@michaelcfanning
Copy link
Contributor

Somehow we appear to have lost the schema constraint that result.message is required. The spec still makes this sensible condition clear.

@ghost ghost added the resolved-by-design label Nov 18, 2018
@ghost
Copy link

ghost commented Nov 18, 2018

@michaelcfanning Both the spec and the schema are correct. The spec says that either message must be present (if the log specifies its messages "inline"), or ruleMessageId must be present (if the log just provides a resource identifier together that locates the message string), or both.

Now even if the log file specifies a message indirectly via ruleMessageId, you still need message if the resource string has any replacement sequences {n} that need to be filled from message.arguments. But if the resource string has no replacement sequences, you don't need message at all.

So the schema can't require message.

@ghost ghost closed this as completed Nov 18, 2018
@ghost ghost self-assigned this Nov 18, 2018
ghost pushed a commit that referenced this issue Nov 18, 2018
@ghost ghost reopened this Nov 20, 2018
@ghost ghost added bug and removed resolved-by-design labels Nov 20, 2018
@ghost
Copy link

ghost commented Nov 20, 2018

This is actually a bug that crept in when we removed result.ruleMessageId. Before then, result.message could not be required, for the reason I stated above. But now, message is required because even a rule's message id is now stored in result.message.messageId, not in a separate result.ruleMessageId.

(The point being, this schema constraint wasn't "dropped"; it was never there. But now it can and should be there.)

@michaelcfanning FYI

@michaelcfanning
Copy link
Contributor Author

Right. I your the reference entirely to a property that we removed.

Great. I can't help but think that our ability to add 'message' as a required property in the schema reflects that the design has improved slightly.

@ghost ghost changed the title result.message SHALL be present constraint dropped from schema result.message SHALL be present constraint should be added to schema Nov 20, 2018
@ghost ghost added 2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. change-draft-superfluous The change is a simple rename; no change draft needed. labels Nov 21, 2018
@ghost
Copy link

ghost commented Nov 21, 2018

This is purely a schema change so no change draft is needed. I'll put it on the agenda to approve it.

ghost pushed a commit that referenced this issue Nov 21, 2018
No change draft needed; schema-only change.
@michaelcfanning
Copy link
Contributor Author

The spec is correct and we should cover this explicitly in the editor's report. I don't see a need to require TC approval on this, however, we simply need to report out on the schema bug.

@ghost
Copy link

ghost commented Nov 23, 2018

Agreed. I'd already removed it from the agenda and added it to the Editor's Report:

  1. I made the following changes at editorial discretion:

    ...

    1. Issue #283: "result.message SHALL be present constraint should be added to schema"

      The spec text already says:

      A result object SHALL contain a property named message...

      It's just a bug that we neglected to mark the property required in the schema. The comment thread in the issue explains how this bug crept in.

      I added this change to the "schema changes" document so that Michael will fix it when he updates the schema with the changes for TC #28.

@michaelcfanning
Copy link
Contributor Author

Updated the schema. Notification and result now both mark 'message' as a required property. @lgolding FYI

@ghost
Copy link

ghost commented Jan 24, 2019

The schema correctly says that result.message is required.

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 change-draft-superfluous The change is a simple rename; no change draft needed. impact-breaks-producers triage-approved
Projects
None yet
Development

No branches or pull requests

1 participant