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

PropertyBag appears to be implemented different from its doc #78

Closed
ZacSweers opened this issue Dec 4, 2023 · 8 comments · Fixed by #101
Closed

PropertyBag appears to be implemented different from its doc #78

ZacSweers opened this issue Dec 4, 2023 · 8 comments · Fixed by #101

Comments

@ZacSweers
Copy link

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

image
@TWiStErRob
Copy link
Member

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
Copy link
Member

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:

  • properties.precision
  • properties.problem.severity
  • properties.security-severity

This was referenced Feb 13, 2024
@BraisGabin
Copy link
Member

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
Copy link
Member

Other option would be to expose Map<String, Any?> and then, under the hood, we can have something like this: Kotlin/kotlinx.serialization#296 (comment)

But it is SO hacky...

@BraisGabin
Copy link
Member

I end up implementing it the "hacky" way.

@TWiStErRob
Copy link
Member

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

@BraisGabin
Copy link
Member

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
Copy link
Author

Lgtm, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants