detekt / sarif4k

Kotlin data bindings for the Static Analysis Results Interchange Format (SARIF)
Apache License 2.0
16 stars 8 forks source link

PropertyBag appears to be implemented different from its doc #78

Closed ZacSweers closed 7 months ago

ZacSweers commented 10 months ago

The doc mentions it's a set of key-value pairs, but it only contains a list of strings

image
TWiStErRob commented 10 months ago

Purely based on the name, it should be something like

class PropertyBag(val tags: List<String>? = null) : HashMap<String, Any?>

based on the schema (search for "propertyBag" incl. quotes).

"propertyBag": {
  "description": "Key/value pairs that provide additional information about the object.",
  "type": "object",
  "additionalProperties": true,
  "properties": {
    "tags": {
      "description": "A set of distinct strings that provide additional information.",
      "type": "array",
      "minItems": 0,
      "uniqueItems": true,
      "default": [],
      "items": {
        "type": "string"
      }
    }
  }
},

makes it a hash + tags is a special typed "key". Might need a custom serializer?

TWiStErRob commented 7 months ago

I just ran into this as well while trying to adopt Sarif that GitHub supports. Without this being implemented, it's not possible to use some properties:

BraisGabin commented 7 months ago

I implement this at #101 but I don't like the API. kotlinx.serialization doesn't allow to use Map<String, Any> so I need to use Map<String, JsonElement>. And that's not ideal because create a JsonElement is a PITA and it also exposes that we use kotlinx.serialization under the hood.

BraisGabin commented 7 months ago

Other option would be to expose Map<String, Any?> and then, under the hood, we can have something like this: https://github.com/Kotlin/kotlinx.serialization/issues/296#issuecomment-1859572541

But it is SO hacky...

BraisGabin commented 7 months ago

I end up implementing it the "hacky" way.

TWiStErRob commented 7 months ago

@ZacSweers do you want to have a look at the public API changes in the PR?

BraisGabin commented 7 months ago

I just merged it but any feedback is still valuable to ensure that it correctly fulfill the needs. I'll try to generate a new release these week.

ZacSweers commented 7 months ago

Lgtm, thanks!