paleobot / pbot-dev

Codebase and initial design documents for pbot client
MIT License
2 stars 2 forks source link

Character Instance Glitch #268

Closed ecurrano closed 4 months ago

ecurrano commented 4 months ago

I am entering Character Instances using the Manual of Leaf Architecture Schema. When I enter a character instance, it shows twice in the character list:

Screen Shot 2024-02-16 at 1 54 38 PM

ecurrano commented 4 months ago

If I delete one of the character instances, they both delete. If I edit the order, both are changed. The example screenshot is for Description Node named "Artocarpus 2 Currano and Wing MLA"

ecurrano commented 4 months ago

When I look the specimen up in Explore, I do see each character instance twice.

NoisyFlowers commented 4 months ago

I can't reproduce this in my local test environment. Before I try it on the prod site, can you tell me a bit more about the problem? You are saying that after you clicked submit for the Lobation/pinnately lobed, order 1, that CharacterInstance appeared twice in the Description? Is it happening for other CharacterInstances you enter or just this one?

ecurrano commented 4 months ago

I've made a key discovery! I had this node as part of two Groups when I was getting the duplications. If I remove one Group, then I only see one CharacterInstance. If I add a second Group back again, then I get the duplication. And when I add it to a third Group, then I see triplication. And this does occur for all CharacterInstances, not just Lobation.

In case it helps, this is description node 3f4fa5e7-6cac-446c-adf5-4db37190b0f5.

NoisyFlowers commented 4 months ago

I haven't tried yet, but this will help.

It also suggests this could be a serious problem. I'm making it high priority.

NoisyFlowers commented 4 months ago

The good news is: this is not a mutation-side problem. Despite seeing multiple duplicate CharacterInstances in the client, they do not exist on the server. The graph there is as it should be with one CharacterInstance inheriting the Group membership of the parent Description. So, the problem is limited to the queries.

The bad news is that the this query problem is deeply tangled up in the way we've hacked neo4j-graphql-js to handle Group security.

Here is the complicated but understandable graphql we run to populate the Description mutation page, including CharacterInstances:

    fragment CharacterFields on Character {
        pbotID  
        name  
        definition  
        order  
        states {
            name    
            __typename  
        }  
        characterInstances(
            filter: {
                description: {
                    pbotID: $descriptionID
                }
            }
        ) {
            pbotID  
            character {
                name
                __typename
            }
            state {
                State {
                    name        
                    pbotID        
                    __typename      
                }      
                order      
                value      
                __typename    
            }    
            __typename  
        }  
        __typename
    }

    fragment CharactersRecurse on Character {
        characters {
            ...CharacterFields    
            characters {
                ...CharacterFields      
                characters {
                    ...CharacterFields        
                        characters {
                            ...CharacterFields          
                            characters {
                                ...CharacterFields            
                                characters {
                                    ...CharacterFields              
                                    __typename            
                                }            
                                __typename          
                            }          
                            __typename        
                        }        
                        __typename      
                }      
                __typename    
            }    
            __typename  
        }  
        __typename
    }

    query ($schemaID: ID, $descriptionID: ID) {
        Schema(pbotID: $schemaID) {
            characters {      
                ...CharacterFields      
                ...CharactersRecurse      
                __typename    
            }    
            __typename  
        }
    }

And here is the cypher generated by neo4j-graphql-js:

MATCH (p:Person {pbotID:"31c0d941-e898-40eb-8885-2e0274ab9bee"})-[:MEMBER_OF]->(g:Group)<-[:ELEMENT_OF|:MEMBER_OF]-(`schema`:`Schema` {pbotID:$`pbotID`}) RETURN DISTINCT `schema` {characters: [(`schema`)<-[:`CHARACTER_OF`]-(`schema_characters`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters` { .pbotID , .name , .definition , .order ,states: [(`schema_characters`)<-[:`STATE_OF`]-(`schema_characters_states`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_states` { .name }] ,characterInstances: [(`schema_characters`)<-[:`INSTANCE_OF`]-(`schema_characters_characterInstances`:`CharacterInstance`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) WHERE (EXISTS((`schema_characters_characterInstances`)<-[:DEFINED_BY]-(:Description)) AND ALL(`description` IN [(`schema_characters_characterInstances`)<-[:DEFINED_BY]-(`_description`:Description) | `_description`] WHERE (`description`.pbotID = $`1_filter`.description.pbotID))) | `schema_characters_characterInstances` { .pbotID ,character: head([(`schema_characters_characterInstances`)-[:`INSTANCE_OF`]->(`schema_characters_characterInstances_character`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characterInstances_character` { .name }]) ,state: head([(`schema_characters_characterInstances`)-[`schema_characters_characterInstances_state_relation`:`HAS_STATE`]->(:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characterInstances_state_relation {State: head([(:`CharacterInstance`)-[`schema_characters_characterInstances_state_relation`]->(`schema_characters_characterInstances_state_State`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characterInstances_state_State { .name , .pbotID }]) , .order , .value }]) }] ,characters: [(`schema_characters`)<-[:`CHARACTER_OF`]-(`schema_characters_characters`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters` {characters: [(`schema_characters_characters`)<-[:`CHARACTER_OF`]-(`schema_characters_characters_characters`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters` {characters: [(`schema_characters_characters_characters`)<-[:`CHARACTER_OF`]-(`schema_characters_characters_characters_characters`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characters` {characters: [(`schema_characters_characters_characters_characters`)<-[:`CHARACTER_OF`]-(`schema_characters_characters_characters_characters_characters`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characters_characters` {characters: [(`schema_characters_characters_characters_characters_characters`)<-[:`CHARACTER_OF`]-(`schema_characters_characters_characters_characters_characters_characters`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characters_characters_characters` {characters: [(`schema_characters_characters_characters_characters_characters_characters`)<-[:`CHARACTER_OF`]-(`schema_characters_characters_characters_characters_characters_characters_characters`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characters_characters_characters_characters` { .pbotID , .name , .definition , .order ,states: [(`schema_characters_characters_characters_characters_characters_characters_characters`)<-[:`STATE_OF`]-(`schema_characters_characters_characters_characters_characters_characters_characters_states`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characters_characters_characters_characters_states` { .name }] ,characterInstances: [(`schema_characters_characters_characters_characters_characters_characters_characters`)<-[:`INSTANCE_OF`]-(`schema_characters_characters_characters_characters_characters_characters_characters_characterInstances`:`CharacterInstance`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) WHERE (EXISTS((`schema_characters_characters_characters_characters_characters_characters_characters_characterInstances`)<-[:DEFINED_BY]-(:Description)) AND ALL(`description` IN [(`schema_characters_characters_characters_characters_characters_characters_characters_characterInstances`)<-[:DEFINED_BY]-(`_description`:Description) | `_description`] WHERE (`description`.pbotID = $`3_filter`.description.pbotID))) | `schema_characters_characters_characters_characters_characters_characters_characters_characterInstances` { .pbotID ,character: head([(`schema_characters_characters_characters_characters_characters_characters_characters_characterInstances`)-[:`INSTANCE_OF`]->(`schema_characters_characters_characters_characters_characters_characters_characters_characterInstances_character`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characters_characters_characters_characters_characterInstances_character` { .name }]) ,state: head([(`schema_characters_characters_characters_characters_characters_characters_characters_characterInstances`)-[`schema_characters_characters_characters_characters_characters_characters_characters_characterInstances_state_relation`:`HAS_STATE`]->(:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characters_characters_characters_characters_characters_characters_characterInstances_state_relation {State: head([(:`CharacterInstance`)-[`schema_characters_characters_characters_characters_characters_characters_characters_characterInstances_state_relation`]->(`schema_characters_characters_characters_characters_characters_characters_characters_characterInstances_state_State`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characters_characters_characters_characters_characters_characters_characterInstances_state_State { .name , .pbotID }]) , .order , .value }]) }] }] , .pbotID , .name , .definition , .order ,states: [(`schema_characters_characters_characters_characters_characters_characters`)<-[:`STATE_OF`]-(`schema_characters_characters_characters_characters_characters_characters_states`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characters_characters_characters_states` { .name }] ,characterInstances: [(`schema_characters_characters_characters_characters_characters_characters`)<-[:`INSTANCE_OF`]-(`schema_characters_characters_characters_characters_characters_characters_characterInstances`:`CharacterInstance`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) WHERE (EXISTS((`schema_characters_characters_characters_characters_characters_characters_characterInstances`)<-[:DEFINED_BY]-(:Description)) AND ALL(`description` IN [(`schema_characters_characters_characters_characters_characters_characters_characterInstances`)<-[:DEFINED_BY]-(`_description`:Description) | `_description`] WHERE (`description`.pbotID = $`3_filter`.description.pbotID))) | `schema_characters_characters_characters_characters_characters_characters_characterInstances` { .pbotID ,character: head([(`schema_characters_characters_characters_characters_characters_characters_characterInstances`)-[:`INSTANCE_OF`]->(`schema_characters_characters_characters_characters_characters_characters_characterInstances_character`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characters_characters_characters_characterInstances_character` { .name }]) ,state: head([(`schema_characters_characters_characters_characters_characters_characters_characterInstances`)-[`schema_characters_characters_characters_characters_characters_characters_characterInstances_state_relation`:`HAS_STATE`]->(:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characters_characters_characters_characters_characters_characterInstances_state_relation {State: head([(:`CharacterInstance`)-[`schema_characters_characters_characters_characters_characters_characters_characterInstances_state_relation`]->(`schema_characters_characters_characters_characters_characters_characters_characterInstances_state_State`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characters_characters_characters_characters_characters_characterInstances_state_State { .name , .pbotID }]) , .order , .value }]) }] }] , .pbotID , .name , .definition , .order ,states: [(`schema_characters_characters_characters_characters_characters`)<-[:`STATE_OF`]-(`schema_characters_characters_characters_characters_characters_states`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characters_characters_states` { .name }] ,characterInstances: [(`schema_characters_characters_characters_characters_characters`)<-[:`INSTANCE_OF`]-(`schema_characters_characters_characters_characters_characters_characterInstances`:`CharacterInstance`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) WHERE (EXISTS((`schema_characters_characters_characters_characters_characters_characterInstances`)<-[:DEFINED_BY]-(:Description)) AND ALL(`description` IN [(`schema_characters_characters_characters_characters_characters_characterInstances`)<-[:DEFINED_BY]-(`_description`:Description) | `_description`] WHERE (`description`.pbotID = $`3_filter`.description.pbotID))) | `schema_characters_characters_characters_characters_characters_characterInstances` { .pbotID ,character: head([(`schema_characters_characters_characters_characters_characters_characterInstances`)-[:`INSTANCE_OF`]->(`schema_characters_characters_characters_characters_characters_characterInstances_character`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characters_characters_characterInstances_character` { .name }]) ,state: head([(`schema_characters_characters_characters_characters_characters_characterInstances`)-[`schema_characters_characters_characters_characters_characters_characterInstances_state_relation`:`HAS_STATE`]->(:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characters_characters_characters_characters_characterInstances_state_relation {State: head([(:`CharacterInstance`)-[`schema_characters_characters_characters_characters_characters_characterInstances_state_relation`]->(`schema_characters_characters_characters_characters_characters_characterInstances_state_State`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characters_characters_characters_characters_characterInstances_state_State { .name , .pbotID }]) , .order , .value }]) }] }] , .pbotID , .name , .definition , .order ,states: [(`schema_characters_characters_characters_characters`)<-[:`STATE_OF`]-(`schema_characters_characters_characters_characters_states`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characters_states` { .name }] ,characterInstances: [(`schema_characters_characters_characters_characters`)<-[:`INSTANCE_OF`]-(`schema_characters_characters_characters_characters_characterInstances`:`CharacterInstance`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) WHERE (EXISTS((`schema_characters_characters_characters_characters_characterInstances`)<-[:DEFINED_BY]-(:Description)) AND ALL(`description` IN [(`schema_characters_characters_characters_characters_characterInstances`)<-[:DEFINED_BY]-(`_description`:Description) | `_description`] WHERE (`description`.pbotID = $`3_filter`.description.pbotID))) | `schema_characters_characters_characters_characters_characterInstances` { .pbotID ,character: head([(`schema_characters_characters_characters_characters_characterInstances`)-[:`INSTANCE_OF`]->(`schema_characters_characters_characters_characters_characterInstances_character`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characters_characterInstances_character` { .name }]) ,state: head([(`schema_characters_characters_characters_characters_characterInstances`)-[`schema_characters_characters_characters_characters_characterInstances_state_relation`:`HAS_STATE`]->(:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characters_characters_characters_characterInstances_state_relation {State: head([(:`CharacterInstance`)-[`schema_characters_characters_characters_characters_characterInstances_state_relation`]->(`schema_characters_characters_characters_characters_characterInstances_state_State`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characters_characters_characters_characterInstances_state_State { .name , .pbotID }]) , .order , .value }]) }] }] , .pbotID , .name , .definition , .order ,states: [(`schema_characters_characters_characters`)<-[:`STATE_OF`]-(`schema_characters_characters_characters_states`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_states` { .name }] ,characterInstances: [(`schema_characters_characters_characters`)<-[:`INSTANCE_OF`]-(`schema_characters_characters_characters_characterInstances`:`CharacterInstance`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) WHERE (EXISTS((`schema_characters_characters_characters_characterInstances`)<-[:DEFINED_BY]-(:Description)) AND ALL(`description` IN [(`schema_characters_characters_characters_characterInstances`)<-[:DEFINED_BY]-(`_description`:Description) | `_description`] WHERE (`description`.pbotID = $`3_filter`.description.pbotID))) | `schema_characters_characters_characters_characterInstances` { .pbotID ,character: head([(`schema_characters_characters_characters_characterInstances`)-[:`INSTANCE_OF`]->(`schema_characters_characters_characters_characterInstances_character`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characters_characterInstances_character` { .name }]) ,state: head([(`schema_characters_characters_characters_characterInstances`)-[`schema_characters_characters_characters_characterInstances_state_relation`:`HAS_STATE`]->(:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characters_characters_characterInstances_state_relation {State: head([(:`CharacterInstance`)-[`schema_characters_characters_characters_characterInstances_state_relation`]->(`schema_characters_characters_characters_characterInstances_state_State`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characters_characters_characterInstances_state_State { .name , .pbotID }]) , .order , .value }]) }] }] , .pbotID , .name , .definition , .order ,states: [(`schema_characters_characters`)<-[:`STATE_OF`]-(`schema_characters_characters_states`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_states` { .name }] ,characterInstances: [(`schema_characters_characters`)<-[:`INSTANCE_OF`]-(`schema_characters_characters_characterInstances`:`CharacterInstance`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) WHERE (EXISTS((`schema_characters_characters_characterInstances`)<-[:DEFINED_BY]-(:Description)) AND ALL(`description` IN [(`schema_characters_characters_characterInstances`)<-[:DEFINED_BY]-(`_description`:Description) | `_description`] WHERE (`description`.pbotID = $`3_filter`.description.pbotID))) | `schema_characters_characters_characterInstances` { .pbotID ,character: head([(`schema_characters_characters_characterInstances`)-[:`INSTANCE_OF`]->(`schema_characters_characters_characterInstances_character`:`Character`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | `schema_characters_characters_characterInstances_character` { .name }]) ,state: head([(`schema_characters_characters_characterInstances`)-[`schema_characters_characters_characterInstances_state_relation`:`HAS_STATE`]->(:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characters_characterInstances_state_relation {State: head([(:`CharacterInstance`)-[`schema_characters_characters_characterInstances_state_relation`]->(`schema_characters_characters_characterInstances_state_State`:`State`)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p) | schema_characters_characters_characterInstances_state_State { .name , .pbotID }]) , .order , .value }]) }] }] }] } AS `schema`

shudder

It took a while to untangle, but here's what's going on.

First, everywhere you see "-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p)" in that cypher, that's a clause we append to insure Group security. We implemented that by hacking neo4j-graphql-js to include that on all nested parts of queries.

After stripping that query down to the bare bones, I came up with this, which reproduces the problem in a more digestable piece:

match 
    (p:Person {pbotID:"31c0d941-e898-40eb-8885-2e0274ab9bee"}),
    (c:Character {pbotID:"2388ecef-8c73-4ed3-a89f-480ce9e34ef0"})<-[:INSTANCE_OF]-(cI:CharacterInstance)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p)
return
    cI

That added clause causes a separate but identical return CharacterInstance for each Group that CharacterInstance and Person share membership in.

This is a problem with the way that clause was implemented. A better way would be a query like this:

match 
    (p:Person {pbotID:"31c0d941-e898-40eb-8885-2e0274ab9bee"}),
    (c:Character {pbotID:"2388ecef-8c73-4ed3-a89f-480ce9e34ef0"})<-[:INSTANCE_OF]-(cI:CharacterInstance) 
        WHERE EXISTS ((cI)-[:ELEMENT_OF|:MEMBER_OF]->()<-[:MEMBER_OF]-(p))
return
    cI

This checks for existence of shared Group membership without returning duplicates. This would be the correct fix. However, implementing it is not so simple. If possible at all, it would require further hacking of neo4j-graphql-js. I'm looking into this now.

There are two alternative fixes. They aren't so much fixes as masks for the problem. But they work and appear to cause no side-effects:

1) We can just filter out duplicates in the client before displaying. This is a one-line change in CharacterAccordion.js. 2) Long ago, I implemented an exclusion list for node types for which we don't want to run the Group check. This is a parameter passed to neo4j-graphql-js, so it doesn't require any internal changes. I could add CharacterInstance to that list. This should be ok from a security perspective, since CharacterInstances are meaningless without the parent Description node.

NoisyFlowers commented 4 months ago

I decided to do the fix described for neo4j-graphql-js.

This is in paleobot/neo4j-graphql-js@3398c28ab5d2444bda3d4ae514060b19984bf551 paleobot/neo4j-graphql-js@1c5d7938be375437aa629a3d0ae8e2ac02e06f17 paleobot/pbot-api@67f26942efa0cb037fe6e2cd6f73b446a787757a paleobot/pbot-api@9cb883a73b1375ab6523ef6a4d976c783e63b461

merged to master and deployed to dev.

IMPORTANT: This is the sort of fix that touches every cypher query/mutation performed by the api. That's scary, and that's why I hate messing with neo4j-graphql-js. I've run it through a bunch of tests, but I'm not going to take this to prod until other eyes can put it through some paces on dev.

NoisyFlowers commented 4 months ago

This is now running on stage/prod