[Zl3h9sM8] apoc.schema.relationships return wrong output for relationship indexes #247
Conversation
vga91
force-pushed
the
fix_apoc.schema.rels_wrong_type_output
branch
from
62aa17a
to
9b4b7bf
Compare
|
|
||
| public IndexConstraintRelationshipInfo(String name, Object type, List<String> properties, String status) { | ||
| public IndexConstraintRelationshipInfo(String name, String type, List<String> properties, String status, Object relationshipType) { |
There was a problem hiding this comment.
can we call this constraintType then it is even less confusing, if you agree, change the one in Nodes as well :)
There was a problem hiding this comment.
Yes, changing the name is a good idea, but maybe I would change the name to schemaType,
because constraintType = INDEX is personally confusing anyway.
It would seem clearer to me even if this change will be made: #247 (review)
There was a problem hiding this comment.
ah fair enough, schemaType works better :) just internally anyway
|
|
||
| public final List<String> properties; | ||
|
|
||
| public final String status; | ||
|
|
||
| public final Object relationshipType; |
There was a problem hiding this comment.
Why is this an Object? (I see it was before as well), but is it not just a string?
There was a problem hiding this comment.
Because relationshipType can also be a List<String>, in case of composite indexes, e.g. in testIndexesWithMultipleLabelsAndRelTypes https://github.com/neo4j/apoc/pull/247/files/9b4b7bf4448cf27d6daff1f1bc9a9d44e23aa11e#diff-231d1cab850aaf45247b0fd9e1a64e18a8002685841df498e2098c38bedf6fb3R758
There was a problem hiding this comment.
Ah makes sense, thanks :)
| } catch (IndexNotFoundKernelException e) { | ||
| return new IndexConstraintRelationshipInfo( | ||
| // Pretty print for index name | ||
| getSchemaInfoName(relName, properties), | ||
| relName, | ||
| "INDEX", |
There was a problem hiding this comment.
above we check: !indexDescriptor.isUnique() ? "INDEX" : "UNIQUENESS", coming very soon is new relationship constraints that allow this, does this check work here too?
There was a problem hiding this comment.
Oh you're right, I didn't remember about your card, I was referring to the current version. Updated :)
| @@ -34,18 +34,18 @@ public class IndexConstraintNodeInfo { | |||
| * @param label | |||
| * @param properties | |||
| * @param status status of the index, if it's a constraint it will be empty | |||
| * @param type if it is an index type will be "INDEX" otherwise it will be the type of constraint | |||
| * @param schemaType if it is an index type will be "INDEX" otherwise it will be the type of constraint | |||
There was a problem hiding this comment.
I realise now this probably changes the actual output? because we probably don't want to do that 🙈
There was a problem hiding this comment.
Actually not, because the output in handled by the field, so public final String type; in this case.
Now I have a doubt, is this what you meant by "internally"?
If not, I can reset it :)
There was a problem hiding this comment.
Oh, you're correct! This is good then :)
…ship indexes (neo4j/apoc#247) * [Zl3h9sM8] apoc.schema.relationships return wrong output for relationship indexes * added getIndexType(indexDescriptor) method * changed attr names
…ship indexes (neo4j/apoc#247) (#3395) * [Zl3h9sM8] apoc.schema.relationships return wrong output for relationship indexes * added getIndexType(indexDescriptor) method * changed attr names
For consistency with the
apoc. schema.nodes, I put"INDEX"instead of relType.I've put the old
"type"in a new field"relationshipType", in order not to lose output info and to be consistent with"label"inschema.nodesproc (I don't know if it can be considered an additional feature).To discuss:
In any case, maybe, in both nodes and relationships procedure, instead of get only "INDEX" we could use
IndexDescriptor.getIndexType()to get a more comprehensive "FULLTEXT"/"LOOKUP"/"TEXT"/"RANGE" or "POINT".Most likely, since it's a feature, this change could be done in another card, in case.