Fix sarif output to use 'match.details' in result object, instead of 'match.message'

[4ch1m]

Hi,

the SARIF output (redundantly) uses the match.message for both the rule and the result objects:

ansible-lint/src/ansiblelint/formatters/__init__.py

Lines 258 to 263 in 4af51fc

	rule: dict[str, Any] = {
	"id": match.tag,
	"name": match.tag,
	"shortDescription": {
	"text": str(match.message),
	},

ansible-lint/src/ansiblelint/formatters/__init__.py

Lines 278 to 282 in 4af51fc

	result: dict[str, Any] = {
	"ruleId": match.tag,
	"message": {
	"text": str(match.message),
	},

This isn't correct, since the (general) rule object should contain the (general) message/description, whereas the result object should provide the detailed description.

This would also be in line with the Codeclimate output (see first and last line):

ansible-lint/src/ansiblelint/formatters/__init__.py

Lines 157 to 173 in 4af51fc

	issue["description"] = self.escape(str(match.message))
	issue["fingerprint"] = hashlib.sha256(
	repr(match).encode("utf-8")
	).hexdigest()
	issue["location"] = {}
	issue["location"]["path"] = self._format_path(match.filename or "")
	if match.column:
	issue["location"]["positions"] = {}
	issue["location"]["positions"]["begin"] = {}
	issue["location"]["positions"]["begin"]["line"] = match.linenumber
	issue["location"]["positions"]["begin"]["column"] = match.column
	else:
	issue["location"]["lines"] = {}
	issue["location"]["lines"]["begin"] = match.linenumber
	if match.details:
	issue["content"] = {}
	issue["content"]["body"] = match.details

[4ch1m]

Hi,
the 'Verify PR label action' fails.
The PR needs an appropriate label.
Is that something the requester can do? Or has this always to be done by the maintainers???

[ssbarnea]

Hi, the 'Verify PR label action' fails. The PR needs an appropriate label. Is that something the requester can do? Or has this always to be done by the maintainers???

Don't worry about that, we add it ourselves. If you using specific semantic branch prefixes like fix/, chore/ or feat/ it can be added automatically by the bot.

[ssbarnea]

Before merging we need to look a little bit at details because AFAIK, some rules might not have details and if true, it might be a regression in UX for these.

If that is true, we might need to conditionally fallback to message.

[4ch1m]

Before merging we need to look a little bit at details

OK.

I stumbled upon this issue while trying to switch from Codeclimate to SARIF output for my IntelliJ plugin; just as you suggested. 😄

Please keep in mind, that at the moment the match.details information is completely absent/lost in the SARIF output.
Which makes the Codeclimate output superior in that regard.
Also meaning, that the SARIF output is no equivalent substitute (UX wise) in my plugin; or for any other similar consumer.

because AFAIK, some rules might not have details and if true, it might be a regression in UX for these.

The PR attaches the match.details to the result object; not the rule object.

[4ch1m]

Hi there,

I just noticed that this PR got marked "incomplete" ("Additional work or information is required").

Is there anything more I can/should provide?

Thanks.

[ssbarnea]

@4ch1m The lack of testing that covers the case where details is the empty string made me mark this as incomplete.

Many rule violations do not have any details (empty string) and this change would negatively impact the outcome for these.

For this reason, we need to ensure we test the behavior for both types of violations, those that have details and those that do not have details, ensuring that the proposed change does not create a case where the errors have no text displayed.

Probably it would make sense to fallback to message when details values as false.

[4ch1m]

That's a good idea; making things bullet-proof. 👍
I've updated the PR.

[4ch1m]

Hmmmm. 🤔

The CI-setup seems to run into "concurrency issues" with my PR.
However, I don't get that when running the tests locally on my machine.

Still haven't figured what's being done here behind the curtains:

    def test_refresh_schemas() -> None:
        """Test for schema update skip."""
        # This is written as a single test in order to avoid concurrency issues,
        # which caused random issues on CI when the two tests run in parallel
        # and or in different order.
        assert refresh_schemas(min_age_seconds=3600 * 24 * 365 * 10) == 0
        sleep(1)
        # this should disable the cache and force an update
>       assert refresh_schemas(min_age_seconds=0) == 1
E       assert 2 == 1
E        +  where 2 = refresh_schemas(min_age_seconds=0)

Any quick hint would be much appreciated. Thanks. 🙇

[4ch1m]

Any quick hint would be much appreciated. Thanks. bow

Nevermind. I just realized this is a general problem with the current codebase. Not related to this PR.