Support custom attributes on nodes and add Neo4j transaction AOP

[Doom9527]

Changelog

Added

Use AOP to manually manage neo4j transactions in the business.

Changed

Support custom attributes on node. Move some common business methods to CommonService(https://github.com/paion-data/aristotle/pull/31/files#diff-f54022a75047358eaeccab8de4837ea72f9040d86d9aa1136df87616b9efa005) to avoid circular calls.

[Doom9527]

The functionality has been developed, but there is a bug that I am working on : springboot @Transactional cannot manage neo4j-java-driver, and abnormal data will not be rolled back. It only works on springboot-neo4j dependencies such as neo4j-spring-boot-starter, and I am considering adding manual transaction management to address this issue.

[QubitPi]

What part of the code made you believe @Transactional is not working as expected?

[Doom9527]

What part of the code made you believe @Transactional is not working as expected?

When I simulated the passing of a duplicate temporaryId, the exception was correctly thrown from src/main/java/com/paiondata/aristotle/service/impl/NodeServiceImpl.java, but the transaction was not rolled back. The code in the data access layer src/main/java/com/paiondata/aristotle/session/NodeCypherRepository.java is not managed by Springboot's transactions.

[QubitPi]

What part of the code made you believe @Transactional is not working as expected?

When I simulated the passing of a duplicate temporaryId, the exception was correctly thrown from src/main/java/com/paiondata/aristotle/service/impl/NodeServiceImpl.java, but the transaction was not rolled back. The code in the data access layer src/main/java/com/paiondata/aristotle/session/NodeCypherRepository.java is not managed by Springboot's transactions.

What database query could even be executed when exception was thrown? This sounds like a bug instead of Spring issue. Can you show me the line of code your are talking about?

[Doom9527]

What part of the code made you believe @Transactional is not working as expected?

When I simulated the passing of a duplicate temporaryId, the exception was correctly thrown from src/main/java/com/paiondata/aristotle/service/impl/NodeServiceImpl.java, but the transaction was not rolled back. The code in the data access layer src/main/java/com/paiondata/aristotle/session/NodeCypherRepository.java is not managed by Springboot's transactions.

What database query could even be executed when exception was thrown? This sounds like a bug instead of Spring issue. Can you show me the line of code your are talking about?

I have found a solution. The situation before was like this: the Neo4j driver opened a session to perform data operations, then closed the session, and later an exception was thrown in the business logic, but the transaction did not roll back. After researching, I learned that transactions can only be managed within the scope of a Neo4j session. Therefore, my solution is to expand the scope of this session. Moreover, Spring's @Transactional annotation is indeed unable to manage the Neo4j driver's session scope; it can only manage database dependencies that are compatible with Spring Boot, such as MyBatis or spring-boot-neo4j-data-starter.

[Doom9527]

One of my sessions looked like this:

public GraphNode createNode(final String graphUuid, final String nodeUuid, final String relationUuid,
                                final String currentTime, final NodeDTO nodeDTO) {
        final StringBuilder setProperties = new StringBuilder();
        for (final Map.Entry<String, String> entry : nodeDTO.getProperties().entrySet()) {
            setProperties.append(", ").append(entry.getKey()).append(": '").append(entry.getValue()).append("'");
        }

        final String cypherQuery = "MATCH (g:Graph) WHERE g.uuid = $graphUuid SET g.update_time = $currentTime "
                + "CREATE (gn:GraphNode{uuid:$nodeUuid "
                + setProperties
                + ",create_time:$currentTime,update_time:$currentTime}) "
                + "WITH g, gn "
                + "CREATE (g)-[r:RELATION {name: 'HAVE', uuid: $relationUuid, "
                + "create_time: $currentTime, update_time: $currentTime}]->(gn) "
                + "RETURN gn";

        try (Session session = driver.session(SessionConfig.builder().build())) {
            return session.writeTransaction(tx -> {
                final var result = tx.run(cypherQuery, Values.parameters(
                                Constants.GRAPH_UUID, graphUuid,
                                Constants.NODE_UUID, nodeUuid,
                                Constants.CURRENT_TIME, currentTime,
                                Constants.RELATION_UUID, relationUuid
                        )
                );

                final Record record = result.next();
                final Map<String, Object> objectMap = Neo4jUtil.extractNode(record.get("gn"));

                return GraphNode.builder()
                        .id((Long) objectMap.get(Constants.ID))
                        .uuid((String) objectMap.get(Constants.UUID))
                        .properties((Map<String, String>) objectMap.get(Constants.PROPERTIES))
                        .createTime((String) objectMap.get(Constants.CREATE_TIME))
                        .updateTime((String) objectMap.get(Constants.UPDATE_TIME))
                        .build();
            });
        }
    }

[Doom9527]

I used an AOP-based transaction mechanism similar to Spring Boot’s @Transactional annotation: I created a custom annotation @Neo4jTransactional. In AOP, I started a transaction for business methods annotated with @Neo4jTransactional, then stored the Transactional object in ThreadLocal. During data insertion in the data access layer, I retrieved the Transactional object from ThreadLocal, thereby managing the entire business method. The specific implementation is in these classes: [src/main/java/com/paiondata/aristotle/aop/Neo4jTransactionAspect.java], [src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java]. If you think this is feasible, I will continue to complete the function of modifying node information and writing tests.

[QubitPi]

I used an AOP-based transaction mechanism similar to Spring Boot’s @Transactional annotation: I created a custom annotation @Neo4jTransactional. In AOP, I started a transaction for business methods annotated with @Neo4jTransactional, then stored the Transactional object in ThreadLocal. During data insertion in the data access layer, I retrieved the Transactional object from ThreadLocal, thereby managing the entire business method. The specific implementation is in these classes: [src/main/java/com/paiondata/aristotle/aop/Neo4jTransactionAspect.java], [src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java]. If you think this is feasible, I will continue to complete the function of modifying node information and writing tests.

That looks OK technically, although unfortunately experiences told me this would be an overkill. I am still having trouble understand this whole issue. This needs more communications.

Could you please highlight the lines of code that broke the transaction mechanism initially? I need more information on this issue. Thanks

By the way, we would cancel the regular meeting this Wednesday in observance of National Day. Take a break and enjoy the holiday. I'll take a look at your code meanwhile.

[Doom9527]

I used an AOP-based transaction mechanism similar to Spring Boot’s @Transactional annotation: I created a custom annotation @Neo4jTransactional. In AOP, I started a transaction for business methods annotated with @Neo4jTransactional, then stored the Transactional object in ThreadLocal. During data insertion in the data access layer, I retrieved the Transactional object from ThreadLocal, thereby managing the entire business method. The specific implementation is in these classes: [src/main/java/com/paiondata/aristotle/aop/Neo4jTransactionAspect.java], [src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java]. If you think this is feasible, I will continue to complete the function of modifying node information and writing tests.

That looks OK technically, although unfortunately experiences told me this would be an overkill. I am still having trouble understand this whole issue. This needs more communications.

Could you please highlight the lines of code that broke the transaction mechanism initially? I need more information on this issue by that. Thanks

Initially, the query in NodeMapperImpl was structured as follows:

public GraphNode createNode(final String graphUuid, final String nodeUuid, final String relationUuid,
                                final String currentTime, final NodeDTO nodeDTO) {
        final StringBuilder setProperties = new StringBuilder();
        for (final Map.Entry<String, String> entry : nodeDTO.getProperties().entrySet()) {
            setProperties.append(", ").append(entry.getKey()).append(": '").append(entry.getValue()).append("'");
        }

        final String cypherQuery = "MATCH (g:Graph) WHERE g.uuid = $graphUuid SET g.update_time = $currentTime "
                + "CREATE (gn:GraphNode{uuid:$nodeUuid "
                + setProperties
                + ",create_time:$currentTime,update_time:$currentTime}) "
                + "WITH g, gn "
                + "CREATE (g)-[r:RELATION {name: 'HAVE', uuid: $relationUuid, "
                + "create_time: $currentTime, update_time: $currentTime}]->(gn) "
                + "RETURN gn";

        try (Session session = driver.session(SessionConfig.builder().build())) {
            return session.writeTransaction(tx -> {
                final var result = tx.run(cypherQuery, Values.parameters(
                                Constants.GRAPH_UUID, graphUuid,
                                Constants.NODE_UUID, nodeUuid,
                                Constants.CURRENT_TIME, currentTime,
                                Constants.RELATION_UUID, relationUuid
                        )
                );

                final Record record = result.next();
                final Map<String, Object> objectMap = Neo4jUtil.extractNode(record.get("gn"));

                return GraphNode.builder()
                        .id((Long) objectMap.get(Constants.ID))
                        .uuid((String) objectMap.get(Constants.UUID))
                        .properties((Map<String, String>) objectMap.get(Constants.PROPERTIES))
                        .createTime((String) objectMap.get(Constants.CREATE_TIME))
                        .updateTime((String) objectMap.get(Constants.UPDATE_TIME))
                        .build();
            });
        }
    }

It opened a new session which was not shared with the session in AOP, leading to an exception where the transaction could not be rolled back after the query. In the new code, NodeMapperImpl[src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java] retrieves the Transactional object pre-placed in ThreadLocal by AOP when initiating the query, which resolves this issue. Initially, I did not use the Spring AOP but instead manually opened a Neo4j session for each business operation that required transaction management, which was not elegant. This approach also required continuously passing the Transactional object through each private method and called methods to ensure that the data access layer could obtain this Transactional object for queries. However, ThreadLocal solved this problem.

[QubitPi]

I used an AOP-based transaction mechanism similar to Spring Boot’s @Transactional annotation: I created a custom annotation @Neo4jTransactional. In AOP, I started a transaction for business methods annotated with @Neo4jTransactional, then stored the Transactional object in ThreadLocal. During data insertion in the data access layer, I retrieved the Transactional object from ThreadLocal, thereby managing the entire business method. The specific implementation is in these classes: [src/main/java/com/paiondata/aristotle/aop/Neo4jTransactionAspect.java], [src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java]. If you think this is feasible, I will continue to complete the function of modifying node information and writing tests.

That looks OK technically, although unfortunately experiences told me this would be an overkill. I am still having trouble understand this whole issue. This needs more communications. Could you please highlight the lines of code that broke the transaction mechanism initially? I need more information on this issue by that. Thanks

Initially, the query in NodeMapperImpl was structured as follows:

public GraphNode createNode(final String graphUuid, final String nodeUuid, final String relationUuid,
                                final String currentTime, final NodeDTO nodeDTO) {
        final StringBuilder setProperties = new StringBuilder();
        for (final Map.Entry<String, String> entry : nodeDTO.getProperties().entrySet()) {
            setProperties.append(", ").append(entry.getKey()).append(": '").append(entry.getValue()).append("'");
        }

        final String cypherQuery = "MATCH (g:Graph) WHERE g.uuid = $graphUuid SET g.update_time = $currentTime "
                + "CREATE (gn:GraphNode{uuid:$nodeUuid "
                + setProperties
                + ",create_time:$currentTime,update_time:$currentTime}) "
                + "WITH g, gn "
                + "CREATE (g)-[r:RELATION {name: 'HAVE', uuid: $relationUuid, "
                + "create_time: $currentTime, update_time: $currentTime}]->(gn) "
                + "RETURN gn";

        try (Session session = driver.session(SessionConfig.builder().build())) {
            return session.writeTransaction(tx -> {
                final var result = tx.run(cypherQuery, Values.parameters(
                                Constants.GRAPH_UUID, graphUuid,
                                Constants.NODE_UUID, nodeUuid,
                                Constants.CURRENT_TIME, currentTime,
                                Constants.RELATION_UUID, relationUuid
                        )
                );

                final Record record = result.next();
                final Map<String, Object> objectMap = Neo4jUtil.extractNode(record.get("gn"));

                return GraphNode.builder()
                        .id((Long) objectMap.get(Constants.ID))
                        .uuid((String) objectMap.get(Constants.UUID))
                        .properties((Map<String, String>) objectMap.get(Constants.PROPERTIES))
                        .createTime((String) objectMap.get(Constants.CREATE_TIME))
                        .updateTime((String) objectMap.get(Constants.UPDATE_TIME))
                        .build();
            });
        }
    }

It opened a new session which was not shared with the session in AOP, leading to an exception where the transaction could not be rolled back after the query. In the new code, NodeMapperImpl[src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java] retrieves the Transactional object pre-placed in ThreadLocal by AOP when initiating the query, which resolves this issue. Initially, I did not use the Spring AOP but instead manually opened a Neo4j session for each business operation that required transaction management, which was not elegant. This approach also required continuously passing the Transactional object through each private method and called methods to ensure that the data access layer could obtain this Transactional object for queries. However, ThreadLocal solved this problem.

When you said "leading to an exception", is it this line Session session = driver.session(SessionConfig.builder().build()) that threw the exception or some other lines of code that did it?

[Doom9527]

I used an AOP-based transaction mechanism similar to Spring Boot’s annotation: I created a custom annotation . In AOP, I started a transaction for business methods annotated with , then stored the object in . During data insertion in the data access layer, I retrieved the object from , thereby managing the entire business method. The specific implementation is in these classes: [src/main/java/com/paiondata/aristotle/aop/Neo4jTransactionAspect.java], [src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java]. If you think this is feasible, I will continue to complete the function of modifying node information and writing tests.@Transactional``@Neo4jTransactional``@Neo4jTransactional``Transactional``ThreadLocal``Transactional``ThreadLocal

That looks OK technically, although unfortunately experiences told me this would be an overkill. I am still having trouble understand this whole issue. This needs more communications. Could you please highlight the lines of code that broke the transaction mechanism initially? I need more information on this issue by that. Thanks

Initially, the query in was structured as follows:NodeMapperImpl

public GraphNode createNode(final String graphUuid, final String nodeUuid, final String relationUuid,
                                final String currentTime, final NodeDTO nodeDTO) {
        final StringBuilder setProperties = new StringBuilder();
        for (final Map.Entry<String, String> entry : nodeDTO.getProperties().entrySet()) {
            setProperties.append(", ").append(entry.getKey()).append(": '").append(entry.getValue()).append("'");
        }

        final String cypherQuery = "MATCH (g:Graph) WHERE g.uuid = $graphUuid SET g.update_time = $currentTime "
                + "CREATE (gn:GraphNode{uuid:$nodeUuid "
                + setProperties
                + ",create_time:$currentTime,update_time:$currentTime}) "
                + "WITH g, gn "
                + "CREATE (g)-[r:RELATION {name: 'HAVE', uuid: $relationUuid, "
                + "create_time: $currentTime, update_time: $currentTime}]->(gn) "
                + "RETURN gn";

        try (Session session = driver.session(SessionConfig.builder().build())) {
            return session.writeTransaction(tx -> {
                final var result = tx.run(cypherQuery, Values.parameters(
                                Constants.GRAPH_UUID, graphUuid,
                                Constants.NODE_UUID, nodeUuid,
                                Constants.CURRENT_TIME, currentTime,
                                Constants.RELATION_UUID, relationUuid
                        )
                );

                final Record record = result.next();
                final Map<String, Object> objectMap = Neo4jUtil.extractNode(record.get("gn"));

                return GraphNode.builder()
                        .id((Long) objectMap.get(Constants.ID))
                        .uuid((String) objectMap.get(Constants.UUID))
                        .properties((Map<String, String>) objectMap.get(Constants.PROPERTIES))
                        .createTime((String) objectMap.get(Constants.CREATE_TIME))
                        .updateTime((String) objectMap.get(Constants.UPDATE_TIME))
                        .build();
            });
        }
    }

It opened a new which was not shared with the in , leading to an exception where the transaction could not be rolled back after the query. In the new code, [src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java] retrieves the Transactional object pre-placed in by AOP when initiating the query, which resolves this issue. Initially, I did not use the but instead manually opened a for each business operation that required transaction management, which was not elegant. This approach also required continuously passing the object through each private method and called methods to ensure that the data access layer could obtain this object for queries. However, solved this problem.session``session``AOP``NodeMapperImpl``ThreadLocal``Spring AOP``Neo4j session``Transactional``Transactional``ThreadLocal

When you said "leading to an exception", is it this line that threw the exception or some other lines of code that did it?Session session = driver.session(SessionConfig.builder().build())

It's an exception in other code that causes the issue. The createNode() method in NodeMapperImpl is called by an upper-level method. If an exception occurs after createNode() finishes executing, the AOP transaction cannot manage createNode().

[QubitPi]

I used an AOP-based transaction mechanism similar to Spring Boot’s annotation: I created a custom annotation . In AOP, I started a transaction for business methods annotated with , then stored the object in . During data insertion in the data access layer, I retrieved the object from , thereby managing the entire business method. The specific implementation is in these classes: [src/main/java/com/paiondata/aristotle/aop/Neo4jTransactionAspect.java], [src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java]. If you think this is feasible, I will continue to complete the function of modifying node information and writing tests.@Transactional@Neo4jTransactional@Neo4jTransactionalTransactionalThreadLocalTransactionalThreadLocal

That looks OK technically, although unfortunately experiences told me this would be an overkill. I am still having trouble understand this whole issue. This needs more communications. Could you please highlight the lines of code that broke the transaction mechanism initially? I need more information on this issue by that. Thanks

Initially, the query in was structured as follows:NodeMapperImpl

public GraphNode createNode(final String graphUuid, final String nodeUuid, final String relationUuid,
                                final String currentTime, final NodeDTO nodeDTO) {
        final StringBuilder setProperties = new StringBuilder();
        for (final Map.Entry<String, String> entry : nodeDTO.getProperties().entrySet()) {
            setProperties.append(", ").append(entry.getKey()).append(": '").append(entry.getValue()).append("'");
        }

        final String cypherQuery = "MATCH (g:Graph) WHERE g.uuid = $graphUuid SET g.update_time = $currentTime "
                + "CREATE (gn:GraphNode{uuid:$nodeUuid "
                + setProperties
                + ",create_time:$currentTime,update_time:$currentTime}) "
                + "WITH g, gn "
                + "CREATE (g)-[r:RELATION {name: 'HAVE', uuid: $relationUuid, "
                + "create_time: $currentTime, update_time: $currentTime}]->(gn) "
                + "RETURN gn";

        try (Session session = driver.session(SessionConfig.builder().build())) {
            return session.writeTransaction(tx -> {
                final var result = tx.run(cypherQuery, Values.parameters(
                                Constants.GRAPH_UUID, graphUuid,
                                Constants.NODE_UUID, nodeUuid,
                                Constants.CURRENT_TIME, currentTime,
                                Constants.RELATION_UUID, relationUuid
                        )
                );

                final Record record = result.next();
                final Map<String, Object> objectMap = Neo4jUtil.extractNode(record.get("gn"));

                return GraphNode.builder()
                        .id((Long) objectMap.get(Constants.ID))
                        .uuid((String) objectMap.get(Constants.UUID))
                        .properties((Map<String, String>) objectMap.get(Constants.PROPERTIES))
                        .createTime((String) objectMap.get(Constants.CREATE_TIME))
                        .updateTime((String) objectMap.get(Constants.UPDATE_TIME))
                        .build();
            });
        }
    }

It opened a new which was not shared with the in , leading to an exception where the transaction could not be rolled back after the query. In the new code, [src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java] retrieves the Transactional object pre-placed in by AOP when initiating the query, which resolves this issue. Initially, I did not use the but instead manually opened a for each business operation that required transaction management, which was not elegant. This approach also required continuously passing the object through each private method and called methods to ensure that the data access layer could obtain this object for queries. However, solved this problem.sessionsessionAOPNodeMapperImplThreadLocalSpring AOPNeo4j sessionTransactionalTransactionalThreadLocal ``

When you said "leading to an exception", is it this line that threw the exception or some other lines of code that did it?Session session = driver.session(SessionConfig.builder().build())

It's an exception in other code that causes the issue. The createNode() method in NodeMapperImpl is called by an upper-level method. If an exception occurs after createNode() finishes executing, the AOP transaction cannot manage createNode().

OK. That cleared things up and I see the big picture now.

Regarding the proposed design, your proposal handles atomicity. Does it handle other aspects of ACID? Will ThreadLocal work for 2 concurrent requests that are mutating the same object given that each request is a thread in Spring?

I've looked at the code and saw your idea clearly. By all means I'm in total support of your plan and I do praise you for proactively handling this hard task other people tend to avoid taking on. 👍🏻

This issue can be addressed partially: replace the HashiCorp entry with the campaign message that "Aristotle supports transactions for graph database". This is a valuable feature and the amount of work behind it won't be subtle.

[Doom9527]

I used an AOP-based transaction mechanism similar to Spring Boot’s annotation: I created a custom annotation . In AOP, I started a transaction for business methods annotated with , then stored the object in . During data insertion in the data access layer, I retrieved the object from , thereby managing the entire business method. The specific implementation is in these classes: [src/main/java/com/paiondata/aristotle/aop/Neo4jTransactionAspect.java], [src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java]. If you think this is feasible, I will continue to complete the function of modifying node information and writing tests.@Neo4jTransactionalTransactionalTransactional@Transactional``@Neo4jTransactional``ThreadLocal``ThreadLocal

That looks OK technically, although unfortunately experiences told me this would be an overkill. I am still having trouble understand this whole issue. This needs more communications. Could you please highlight the lines of code that broke the transaction mechanism initially? I need more information on this issue by that. Thanks

Initially, the query in was structured as follows:NodeMapperImpl

public GraphNode createNode(final String graphUuid, final String nodeUuid, final String relationUuid,
                                final String currentTime, final NodeDTO nodeDTO) {
        final StringBuilder setProperties = new StringBuilder();
        for (final Map.Entry<String, String> entry : nodeDTO.getProperties().entrySet()) {
            setProperties.append(", ").append(entry.getKey()).append(": '").append(entry.getValue()).append("'");
        }

        final String cypherQuery = "MATCH (g:Graph) WHERE g.uuid = $graphUuid SET g.update_time = $currentTime "
                + "CREATE (gn:GraphNode{uuid:$nodeUuid "
                + setProperties
                + ",create_time:$currentTime,update_time:$currentTime}) "
                + "WITH g, gn "
                + "CREATE (g)-[r:RELATION {name: 'HAVE', uuid: $relationUuid, "
                + "create_time: $currentTime, update_time: $currentTime}]->(gn) "
                + "RETURN gn";

        try (Session session = driver.session(SessionConfig.builder().build())) {
            return session.writeTransaction(tx -> {
                final var result = tx.run(cypherQuery, Values.parameters(
                                Constants.GRAPH_UUID, graphUuid,
                                Constants.NODE_UUID, nodeUuid,
                                Constants.CURRENT_TIME, currentTime,
                                Constants.RELATION_UUID, relationUuid
                        )
                );

                final Record record = result.next();
                final Map<String, Object> objectMap = Neo4jUtil.extractNode(record.get("gn"));

                return GraphNode.builder()
                        .id((Long) objectMap.get(Constants.ID))
                        .uuid((String) objectMap.get(Constants.UUID))
                        .properties((Map<String, String>) objectMap.get(Constants.PROPERTIES))
                        .createTime((String) objectMap.get(Constants.CREATE_TIME))
                        .updateTime((String) objectMap.get(Constants.UPDATE_TIME))
                        .build();
            });
        }
    }

It opened a new which was not shared with the in , leading to an exception where the transaction could not be rolled back after the query. In the new code, [src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java] retrieves the Transactional object pre-placed in by AOP when initiating the query, which resolves this issue. Initially, I did not use the but instead manually opened a for each business operation that required transaction management, which was not elegant. This approach also required continuously passing the object through each private method and called methods to ensure that the data access layer could obtain this object for queries. However, solved this problem.sessionNodeMapperImplSpring AOPTransactionalThreadLocal ` sessionAOPThreadLocalNeo4j session`Transactional

When you said "leading to an exception", is it this line that threw the exception or some other lines of code that did it?Session session = driver.session(SessionConfig.builder().build())

It's an exception in other code that causes the issue. The method in is called by an upper-level method. If an exception occurs after finishes executing, the AOP transaction cannot manage .createNode()``NodeMapperImpl``createNode()``createNode()

OK. That cleared things up and I see the big picture now.

Regarding the proposed design, your proposal handles atomicity. Does it handle other aspects of ACID? Will work for 2 concurrent requests that are mutating the same object given that each request is a thread in Spring?ThreadLocal

I've looked at the code and saw your idea clearly. By all means I'm in total support of your plan and I do praise you for proactively handling this hard task other people tend to avoid taking on. 👍🏻

This issue can be addressed partially: replace the HashiCorp entry with the campaign message that "Aristotle supports transactions for graph database". This is a valuable feature and the amount of work behind it won't be subtle.

It ensures every aspect of ACID.

Additionally, according to the official Neo4j documentation, due to the default level being read-committed isolation level, deadlocks can occur when updating the same object concurrently.

[QubitPi]

I used an AOP-based transaction mechanism similar to Spring Boot’s annotation: I created a custom annotation . In AOP, I started a transaction for business methods annotated with , then stored the object in . During data insertion in the data access layer, I retrieved the object from , thereby managing the entire business method. The specific implementation is in these classes: [src/main/java/com/paiondata/aristotle/aop/Neo4jTransactionAspect.java], [src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java]. If you think this is feasible, I will continue to complete the function of modifying node information and writing tests.@Neo4jTransactionalTransactionalTransactional@Transactional@Neo4jTransactionalThreadLocalThreadLocal ``

That looks OK technically, although unfortunately experiences told me this would be an overkill. I am still having trouble understand this whole issue. This needs more communications. Could you please highlight the lines of code that broke the transaction mechanism initially? I need more information on this issue by that. Thanks

Initially, the query in was structured as follows:NodeMapperImpl

public GraphNode createNode(final String graphUuid, final String nodeUuid, final String relationUuid,
                                final String currentTime, final NodeDTO nodeDTO) {
        final StringBuilder setProperties = new StringBuilder();
        for (final Map.Entry<String, String> entry : nodeDTO.getProperties().entrySet()) {
            setProperties.append(", ").append(entry.getKey()).append(": '").append(entry.getValue()).append("'");
        }

        final String cypherQuery = "MATCH (g:Graph) WHERE g.uuid = $graphUuid SET g.update_time = $currentTime "
                + "CREATE (gn:GraphNode{uuid:$nodeUuid "
                + setProperties
                + ",create_time:$currentTime,update_time:$currentTime}) "
                + "WITH g, gn "
                + "CREATE (g)-[r:RELATION {name: 'HAVE', uuid: $relationUuid, "
                + "create_time: $currentTime, update_time: $currentTime}]->(gn) "
                + "RETURN gn";

        try (Session session = driver.session(SessionConfig.builder().build())) {
            return session.writeTransaction(tx -> {
                final var result = tx.run(cypherQuery, Values.parameters(
                                Constants.GRAPH_UUID, graphUuid,
                                Constants.NODE_UUID, nodeUuid,
                                Constants.CURRENT_TIME, currentTime,
                                Constants.RELATION_UUID, relationUuid
                        )
                );

                final Record record = result.next();
                final Map<String, Object> objectMap = Neo4jUtil.extractNode(record.get("gn"));

                return GraphNode.builder()
                        .id((Long) objectMap.get(Constants.ID))
                        .uuid((String) objectMap.get(Constants.UUID))
                        .properties((Map<String, String>) objectMap.get(Constants.PROPERTIES))
                        .createTime((String) objectMap.get(Constants.CREATE_TIME))
                        .updateTime((String) objectMap.get(Constants.UPDATE_TIME))
                        .build();
            });
        }
    }

It opened a new which was not shared with the in , leading to an exception where the transaction could not be rolled back after the query. In the new code, [src/main/java/com/paiondata/aristotle/mapper/impl/NodeMapperImpl.java] retrieves the Transactional object pre-placed in by AOP when initiating the query, which resolves this issue. Initially, I did not use the but instead manually opened a for each business operation that required transaction management, which was not elegant. This approach also required continuously passing the object through each private method and called methods to ensure that the data access layer could obtain this object for queries. However, solved this problem.sessionNodeMapperImplSpring AOPTransactionalThreadLocal ` session`AOP`ThreadLocalNeo4j session`Transactional

When you said "leading to an exception", is it this line that threw the exception or some other lines of code that did it?Session session = driver.session(SessionConfig.builder().build())

It's an exception in other code that causes the issue. The method in is called by an upper-level method. If an exception occurs after finishes executing, the AOP transaction cannot manage .createNode()NodeMapperImplcreateNode()createNode() ``

OK. That cleared things up and I see the big picture now. Regarding the proposed design, your proposal handles atomicity. Does it handle other aspects of ACID? Will work for 2 concurrent requests that are mutating the same object given that each request is a thread in Spring?ThreadLocal I've looked at the code and saw your idea clearly. By all means I'm in total support of your plan and I do praise you for proactively handling this hard task other people tend to avoid taking on. 👍🏻 This issue can be addressed partially: replace the HashiCorp entry with the campaign message that "Aristotle supports transactions for graph database". This is a valuable feature and the amount of work behind it won't be subtle.

It ensures every aspect of ACID.

Additionally, according to the official Neo4j documentation, due to the default level being read-committed isolation level, deadlocks can occur when updating the same object concurrently.

I see. So you are threadlocaling sequential memory which should be fine in this case. My apology.

On the other hand, if every request spawns a single thread only, how about storing everything in plain object and passing it around instead of ThreadLocal? Introducing async concept in a synchronous scenario is an overkill and very hard to test. A simple object that carries information around would be more than enough.

I understand ThreadLocal looks cool to use. Unless we have no alternative, ThreadLocal/asynchronism, however, makes code very hard to read, test, and debug later on (true story: I had hell lot of pain on this in the past). Industrial code is not about fanciness but simplify with efficiency, readability, and maintainability. You will become more agreeable to this claim as you gain more experience through your career.

In addition, when you say "It ensures every aspect of ACID, period", it really means nothing to others. Why does it sure every aspect of ACID? You need to justify your claim. Otherwise people have no choice but only to disagree with you. I'm helping you to avoid potential frictions with colleagues in your future work and believe me - more communications always help. Don't worry, this takes time to practice.

[Doom9527]

Atomicity: In code, the manageTransaction method ensures that if an exception occurs during the method execution, the transaction will be rolled back. This aligns with the requirements of atomicity. Consistency: From the code , I check whether nodes have been bound to another graph in the checkInputRelationsAndBindGraphAndNode method, which can prevent inconsistency issues. Isolation: Neo4j supports multiple transaction isolation levels by default. Using Neo4j's default settings, it should provide sufficient isolation to prevent interference between transactions. Durability: When the commitTransaction(tx) method is called, the changes made by the transaction are committed and logged to ensure durability.

After reading your comments, I agree that using ThreadLocal can indeed bring many troubles. Now, I have removed it and explicitly pass the transaction in the method calls. The Neo4jTransactionAspect now dynamically passes a Transaction object to the business methods marked with the @Neo4jTransactional annotation. The Neo4jServiceImpl will obtain this object and then pass it to the data layer. This way, it also ensures that the entire business operation is managed by the Neo4j transaction.

[QubitPi]

Regarding checkstyle complaining about Exception e, there are two options:

1 - Go Aggressive

Delete the IllegalCatch entry in checkstyle and simply legitimate the Exception.

This approach favors the startup-style project where things are implemented fast with the cost of more maintaining efforts later - more bugs and harder debugging. But allowing Exception shouldn't be too much of a big deal.

2 - More Efforts Now

There are several reasons why catching Exception is not acceptable:

1. Exception allows bug masking. If we have a piece of code which is not working well and throwing exceptions (or we pass malformed input to that piece of code) and we just blind our eyes by catching all possible exceptions, we will actually never uncover the bug and fix it. This gives extreme pain when maintaining a software, a phase where you haven't been exposed to too much, which is why Exception seems OK to you
2. If we decide to catch an exception, we must then handle it. Exception really gives us no clue on what just happened, making it impossible for us to decide that error message and higher-level exceptions to bubble up.

Both Are Correct

I'm sure the code can be refactored to avoid the Exception, but I need to mention that option 1 is more like yahoo/Elide style focusing on speed and startup project; option 2 is more like yahoo/fili style and lean more towards software perfectionism in a more stable team. I've worked in both projects before and know both are good and bad in different ways.

Which one then? I recommend option 1, but if you are interested in this problem, I am more than happy to work out a better design with you using option 2

[Doom9527]

Regarding checkstyle complaining about , there are two options:Exception e

1 - Go Aggressive

Delete the entry in checkstyle and simply legitimate the .IllegalCatch``Exception

This approach favors the startup-style project where things are implemented fast with the cost of more maintaining efforts later - more bugs and harder debugging. But allowing shouldn't be too much of a big deal.Exception

2 - More Efforts Now

There are several reasons why catching is not acceptable:Exception

1. Exception allows bug masking. If we have a piece of code which is not working well and throwing exceptions (or we pass malformed input to that piece of code) and we just blind our eyes by catching all possible exceptions, we will actually never uncover the bug and fix it. This gives extreme pain when maintaining a software, a phase where you haven't been exposed to too much, which is why Exception seems OK to you
2. If we decide to an exception, we must then handle it. really gives us no clue on what just happened, making it impossible for us to decide that error message and higher-level exceptions to bubble up.catch``Exception

Both Are Correct

I'm sure the code can be refactored to avoid the , but I need to mention that option 1 is more like yahoo/Elide style focusing on speed and startup project; option 2 is more like yahoo/fili style and lean more towards software perfectionism in a more stable team. I've worked in both projects before and know both are good and bad in different ways.Exception

Which one then? I recommend option 1, but if you are interested in this problem, I am more than happy to work out a better design with you using option 2

For option 2, it is considered necessary to manually catch all possible exceptions, and these exceptions are all custom exceptions that I have defined. However, when developing new features subsequently, we would need to modify the aspect class to add new custom exceptions. I believe it might be necessary to open a new issue to address this problem. Currently, we need to finalize this PR as soon as possible since its changes are too extensive and are blocking subsequent tasks.

[QubitPi]

Regarding checkstyle complaining about , there are two options:Exception e

1 - Go Aggressive

Delete the entry in checkstyle and simply legitimate the .IllegalCatchException ` This approach favors the startup-style project where things are implemented fast with the cost of more maintaining efforts later - more bugs and harder debugging. But allowing shouldn't be too much of a big deal.Exception`

2 - More Efforts Now

There are several reasons why catching is not acceptable:Exception

1. Exception allows bug masking. If we have a piece of code which is not working well and throwing exceptions (or we pass malformed input to that piece of code) and we just blind our eyes by catching all possible exceptions, we will actually never uncover the bug and fix it. This gives extreme pain when maintaining a software, a phase where you haven't been exposed to too much, which is why Exception seems OK to you
2. If we decide to an exception, we must then handle it. really gives us no clue on what just happened, making it impossible for us to decide that error message and higher-level exceptions to bubble up.catchException ``

Both Are Correct

I'm sure the code can be refactored to avoid the , but I need to mention that option 1 is more like yahoo/Elide style focusing on speed and startup project; option 2 is more like yahoo/fili style and lean more towards software perfectionism in a more stable team. I've worked in both projects before and know both are good and bad in different ways.Exception Which one then? I recommend option 1, but if you are interested in this problem, I am more than happy to work out a better design with you using option 2

For option 2, it is considered necessary to manually catch all possible exceptions, and these exceptions are all custom exceptions that I have defined. However, when developing new features subsequently, we would need to modify the aspect class to add new custom exceptions. I believe it might be necessary to open a new issue to address this problem. Currently, we need to finalize this PR as soon as possible since its changes are too extensive and are blocking subsequent tasks.

I fully agree. Feel free to open up an issue

[Doom9527]

👍🏻 Good work! Ready to go.

I will roll out details about filtering and pagination tomorrow.

Feel free to pick up any issue as the next task. They are equally important and, at this moment, equally urgent.

I will next choose to address either the pagination and filtering issue or issue #20, where the Master CI/CD should trigger an acceptance test run.