ShiftLeftSecurity / codepropertygraph

Code Property Graph: specification, query language, and utilities
Apache License 2.0
460 stars 73 forks source link

Incorrect cardinality for NodeKeys.java // INHERITS_FROM_TYPE_FULL_NAME #429

Closed bbrehm closed 5 years ago

bbrehm commented 5 years ago

The generated NodeKeys.java contains

public static final Key<String> INHERITS_FROM_TYPE_FULL_NAME = new Key<>("INHERITS_FROM_TYPE_FULL_NAME");

This is incorrect: It should be a list of strings.

This trips up vertex.value2(NodeKeys.INHERITS_FROM_TYPE_FULL_NAME) in fuzzyc2cpg.

mpollmeier commented 5 years ago

You hit another quirky point of the tinkerpop api. Unfortunately setting and getting properties is asymmetric with regards to types. When setting a property, one has to provide it with the base type (e.g. String), independent of it's cardinality. This will set a single property:

v.property("name", "foo")

while this will make it a multi-property and append to a list (calling the same again will append)

v.property(Cardinality.list, "name", "foo")

However, when reading a property, you will get the full type, i.e. String for a single property, but List[String] for a multi-property.

There's some examples in http://tinkerpop.apache.org/docs/current/reference/ but that's the gist ^

Long story short: yes it's not ideal, and there may be other workarounds, but I just made some simple suggestions on the PR you linked from here.

bbrehm commented 5 years ago

I see, so the key types are as intended correct and the signature value2: Key[T]->Tis incorrect? Because we can also get back an iterable of T?

Do we want to fix that or keep the type annotations unsound? Do we want an immediate error with helpful suggestion on value2(some_key) for all broken keys?

("This key represents a property that can have higher cardinality. value2 cannot deal with that. Use value3 instead")

Thanks for your suggestions!

mpollmeier commented 5 years ago

value3 :woman_facepalming: :) reminds me how silly value2 is

Because if it's (ugly) asymmetric nature there's no clean solution for this unfortunately, so we'll keep it as is for now and will do a better job when using the overflowdb-traversal language.