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

Define a reportingDescriptorReference object #324

Closed
michaelcfanning opened this issue Feb 20, 2019 · 7 comments
Closed

Define a reportingDescriptorReference object #324

michaelcfanning opened this issue Feb 20, 2019 · 7 comments
Labels
2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. design-improvement e-ballot e-ballot-3 impact-breaks-consumers impact-breaks-producers merged Changes merged into provisional draft. p1 Priority 1 issue to close resolved-fixed tc-32 tc-33 Issues to present at SARIF TC33

Comments

@michaelcfanning
Copy link
Contributor

michaelcfanning commented Feb 20, 2019

EBALLOT PROPOSAL. Define a reporting descriptor reference object that allows notifications to point into reporting metadata that may be persisted to the driver or any of its extensions or to the new taxonomies metadata.

API IMPACT

Schema only PR: microsoft/sarif-sdk#1316

  • Create a reportingDescriptorReference type with these properties.
    • id of type string: a notification identifier
    • pointer of type string: a JSON pointer that locates the descriptor for the notification with that identifier.
  • In the reportingConfigurationOverride object:
    • Remove the notificationIndex, ruleIndex, and extensionIndex properties.
    • Replace them with a reportingDescriptorReference property of type reportingDescriptorReference, which specifies the reportingDescriptor whose configuration is being overridden.
  • In the notification object:
    • Remove the id property.
    • Replace it with a notificationDescriptorReference property of type reportingDescriptorReference.
    • Remove the ruleId and ruleIndex properties.
    • Replace them with an associatedRuleDescriptorReference property of type reportingDescriptorReference, which locates the descriptor for the rule, if any, with which this notification is associated.
  • In the invocation object:
    • Replace the property reportingConfigurationOverrides (which was defined to hold overrides for both rules and notifications) with separate properties ruleConfigurationOverrides and notificationConfigurationOverrides.
  • In the result object
    • Remove the ruleIndex and extensionIndex properties.
    • Replace them with a rulePointer property of type string, containing a JSON pointer that locates the rule descriptor.

EXAMPLE

{                            # A run object
  "tool": {
    "driver": {
      "name": "CodeScanner",
      "ruleDescriptors": [
        {                    # A reportingDescriptor object.
          "id": "CA2101",
          "name": "DoNotBeEvil",
          "shortDescription": {
            "text": "Do unto others as you would have them do unto you."
          },
          "messageStrings": {
            "default": {
              "text": "Evildoing was detected."
            }
          },
          "defaultConfiguration": {
            "level": "error"
          }
        }
      ],
      "notificationDescriptors": [
        {
          "id": "MSG0001",
          "name": "RuleDisabled",
          "shortDescription": {
            "text": "This notification occurs when a rule is disabled due to an exception."
          },
          "messageStrings": {
            "default": {
              "text": "Rule {0} has been disabled."
            }
          }
        }
      ]
    }
  },
  "results": [
    {                         # A result object.
      "id": "CA2101",
      "rulePointer":  "0"     # In this context, an abbreviation for "driver/ruleDescriptors/0"
      "message": {
        "messageId": "default"
      }
    }
  ],
  "invocations": [
    {
      "ruleConfigurationOverrides": [
        {
          "reportingDescriptorReference": {
            "id": "CA2101"
            "pointer": "0"     # In this context, ruleDescriptors are the default target
          },
          "configuration": {
            "level": "warning"
          }
        }
      ],
      "toolNotifications": [  # toolExecutionNotifications if #330 is approved
        {
          "notificationDescriptorReference": {
            "id": "MSG0001",
            "pointer": "0"    # In this context, an abbreviation for "driver/notificationDescriptors/0"
          },
          "associatedRuleDescriptorReference": {
            "id": "CA2101",
            "pointer": "0"    # In this context, an abbreviation for "driver/ruleDescriptors/0"
          }
          "message": {
            "messageId": "default",
            "arguments": [
              "CA2101"
            ]
          }
        }
      ]
    }
  ]
}

NOTES

  1. Certain JSON pointer-valued properties can be abbreviated, to optimize the most common cases:
  • A JSON pointer that refers to a descriptor defined by the tool's driver can be abbreviated to a stringified integer value >=0 that will be treated as a relative reference into the driver's ruleDescriptors array or notificationDescriptors array, as appropriate. That is, if notification.notificationDescriptorReference is "driver/notificationDescriptors/42", it can be abbreviated to "42", and if notification.associatedRuleDescriptorReference is "driver/ruleDescriptors/54", it can be abbreviated to "54".
  1. We do not replace result.ruleId and result.rulePointer with a reportingDescriptorReference in order to minimize churn in SARIF pre-release v2 producers.

  2. taxonomies will also use this mechanism. That is covered in Define result taxonomies #314.

@oasis-tcs oasis-tcs deleted a comment Feb 27, 2019
@oasis-tcs oasis-tcs deleted a comment Feb 27, 2019
@michaelcfanning michaelcfanning added 2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. e-ballot labels Feb 27, 2019
@ghost ghost changed the title Consider a 'notification locator' object Define a reportingDescriptorReference object Feb 27, 2019
michaelcfanning added a commit to microsoft/sarif-sdk that referenced this issue Feb 28, 2019
@kupsch
Copy link

kupsch commented Mar 7, 2019

introducing a microlanguage where the value of the DescriptorReference determines what the relative URI is relative to unnecessarily complicates things. Just use a prefix of ../.. to get to the tool object.

@kupsch
Copy link

kupsch commented Mar 7, 2019

I think eliminating the pointer all together, along with changing the descriptor arrays to an object with the keys being an the guid for the notification would be a better design. Then you just lookup the id in the driver and extensions for a match.

@michaelcfanning
Copy link
Contributor Author

michaelcfanning commented Mar 7, 2019

Based on Jim's feedback, we propose to rework the reportingDescriptorReference as follows:

  • id : the id of the descriptor
  • index: the index into an array of descriptors
  • guid: a guid that uniquely identifies the descriptor.
  • toolComponentIndex: the index of the tool component in tool.extensions.
  • toolComponentGuid: a guid that uniquely identifies the tool component associated with the descriptor

Notes:

  1. All these properties will be inlined into the result object, as with today's design, to minimize breaking changes.
  2. index on its own references into tool.driver descriptors arrays. for extensions and other uncommon cases, the toolComponentGuid must be used to locate the relevant descriptors array (which is indexed into via the index property.
  3. If the descriptorGuid is present, the consumer must potentially consult all descriptor data in the log file to locate it. The index and toolComponentGuid if present may be disregarded in this case.

@michaelcfanning michaelcfanning added the tc-33 Issues to present at SARIF TC33 label Mar 7, 2019
@ghost ghost added the e-ballot-3 label Mar 18, 2019
@michaelcfanning
Copy link
Contributor Author

Closing this issue. We will accept changes to this type in context of the open taxonomies proposal (where it is utilized). #314

@michaelcfanning
Copy link
Contributor Author

Reopening, as other proposals don't clearly/comprehensively update the `reportingDescriptorReference' type.

@ghost
Copy link

ghost commented Mar 22, 2019

E-BALLOT #3 PROPOSAL

Define a reportingDescriptorReference object that allows results, notifications, and taxonomy references (#314) to point into reporting metadata that may reside in the driver or any of its extensions.

SCHEMA CHANGES

  • Define a toolComponentReference object with the following properties:

    • name of type string, optional (matches the name property of the referenced toolComponent).
    • index of type integer, optional, default: -1, minValue: -1 (the index of the referenced toolComponent in tool.extensions, if that's where it lives).
    • guid of type string, optional (matches the guid property of the referenced toolComponent object).
  • Define a reportingDescriptorReference object with the following properties:

    • id of type string, optional (matches the id property of the referenced reportingDescriptor).
    • index of type integer, optional, default: -1, minValue: -1 (the index of the referenced reportingDescriptor in toolComponent.ruleDescriptors, toolComponent.notificationDescriptors, or toolComponent.taxonomyDescriptors (Define result taxonomies #314), depending on context).
    • guid of type string, optional (matches the guid property of the referenced reportingDescriptor).
    • toolComponentReference of type toolComponentReference, optional.
  • Define a reportingConfigurationOverride object with the following properties:

    • reportingDescriptorReference of type reportingDescriptorReference, required: specifies the reportingDescriptor whose configuration is being overridden.
    • configuration of type reportingConfiguration, required: specifies the overridden configuration values.
  • In the result object:

    • Add a property ruleDescriptorReference of type reportingDescriptorReference, optional.
    • Remove the ruleExtensionIndex property.
    • For backwards compatibility, retain the ruleId and ruleIndex properties.
  • In the notification object:

    • Add a property notificationDescriptorReference of type reportingDescriptorReference, optional.
    • Add a property associatedRuleDescriptorReference of type reportingDescriptorReference, optional.
    • Remove the id, ruleId, and ruleIndex properties.
  • In the invocation object:

    • Add a property ruleConfigurationOverrides of type reportingConfigurationOverride[], optional, uniqueItems, minItems: 0, default: empty array.
    • Add a property notificationConfigurationOverrides of type reportingConfigurationOverride[], optional, uniqueItems, minItems: 0, default: empty array.

ghost pushed a commit that referenced this issue Mar 22, 2019
ghost pushed a commit that referenced this issue Mar 23, 2019
ghost pushed a commit that referenced this issue Mar 23, 2019
@ghost ghost self-assigned this Mar 23, 2019
@ghost ghost added p1 Priority 1 issue to close merged Changes merged into provisional draft. e-ballot-3 and removed resolved-duplicate labels Mar 23, 2019
@ghost
Copy link

ghost commented Apr 6, 2019

Approved in e-ballot-3.

@ghost ghost closed this as completed Apr 6, 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. design-improvement e-ballot e-ballot-3 impact-breaks-consumers impact-breaks-producers merged Changes merged into provisional draft. p1 Priority 1 issue to close resolved-fixed tc-32 tc-33 Issues to present at SARIF TC33
Projects
None yet
Development

No branches or pull requests

2 participants