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

Neo4j relation rework #13106

Merged
merged 12 commits into from
Jan 4, 2021
Merged

Neo4j relation rework #13106

merged 12 commits into from
Jan 4, 2021

Conversation

Evertude
Copy link
Contributor

When a Neo4J entity has relations, two findAll methods are created in the service: One without parameters and one with a Pageable.

    /**
     * Get all the items.
     *
     * @return the list of entities.
     */
    public List<Item> findAll() {
        log.debug("Request to get all Items");
        return itemRepository.findAllWithEagerRelationships();
    }

    /**
     * Get all the items with eager load of many-to-many relationships.
     *
     * @return the list of entities.
     */
    public Page<Item> findAllWithEagerRelationships(Pageable pageable) {
        return itemRepository.findAllWithEagerRelationships(pageable);
    }

Currently there is no findall method in the repository without parameters so it is added in this PR.


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@CLAassistant
Copy link

CLAassistant commented Nov 20, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@DanielFran DanielFran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Evertude Can you add tests please?

@Evertude
Copy link
Contributor Author

I'm currently reworking the whole neo4j entity creation process because it doesn't work (for me) so it might take a bit.

I'm not familiar with your testing infrastructure but as far as I understood, the tests only involve "Files are created".
Do you have any code that compiles the created projects and runs the tests within them?

@atomfrede
Copy link
Member

I think Daniel is referring to a generated tests in the application. So the integration tests will have a tests making sure everything works as expected (can't see the code as I am on my Mobile).

@Evertude
Copy link
Contributor Author

Well, the tests already exist. The problem is that the code doesn't compile in the first place.

…into main

# Conflicts:
#	generators/entity-server/templates/src/main/java/package/repository/EntityRepository.java.ejs
@Evertude Evertude changed the title Neo4j add missing eager find all without pageable Neo4j relation rework Nov 30, 2020
@Evertude
Copy link
Contributor Author

Finished the relationship rework.

Before:
One-to-many relations don't create directed database relations and instead create relation loops between nodes
This results in recursions when querying data which lead to heap overflow

After:
Relations are properly directed and use Neo4J naming scheme

Additionaly:
Removed the eager loading stuff to avoid confusion because it is the default with Spring Data Neo4j

@Evertude
Copy link
Contributor Author

related to jhipster/jhipster-bom#79

@@ -285,10 +285,16 @@ public class <%= asEntity(entityClass) %> implements Serializable {
}

relationships.forEach((relationship, idx) => {
const otherEntityRelationshipName = relationships[idx].otherEntityRelationshipName;
const otherEntityRelationshipName =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a different nomenclature for relationship name, the rule should be applied where it is calculated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this nomenclature should only apply to the database mapping itself and this is the first time it is calculated

https://neo4j.com/docs/cypher-manual/current/syntax/naming/#_recommendations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise it would be also used in the front end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Evertude I was talking to calculate it directly on function prepareRelationshipForTemplates, but @mashima can confirm if it is the correct function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mshima @atomfrede If we re-calculate this relationship name here, this can be an issue due to the size of the relationship length (we are adding more 4 characters HAL_)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard to say, this is a neo4j exclusive feature and there is no neo4j test workflow to see if it works.
I know for sure that those won't be propagated to others generators and templates, and changing the main relationshipName will break others generators like frontend.
I will let neo4j experts to decide.

const otherEntityRelationshipNamePlural = relationships[idx].otherEntityRelationshipNamePlural;
const otherEntityIsEmbedded = relationships[idx].otherEntityIsEmbedded;
const relationshipName = relationships[idx].relationshipName;
const relationshipName =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a different nomenclature for relationship name, the rule should be applied where it is calculated?

@@ -633,7 +633,9 @@ class EntityGenerator extends BaseBlueprintGenerator {
(this.context.eagerLoad ||
(this.context.paginate !== 'pagination' &&
relationship.relationshipType === 'many-to-many' &&
relationship.ownerSide === true));
relationship.ownerSide === true)) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition should be OR this.context.databaseType === 'neo4j'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the default for Spring Data Neo4J is eager loading, I thought it doesn't make sense to create additional code like findAllWithEager which this flag determines. That's why I turned it to false if the databaseType is neo4j

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative would be to override the default findAll method with lazy loading and add the eager loading ones. Could do that if you want me to

@@ -285,10 +285,16 @@ public class <%= asEntity(entityClass) %> implements Serializable {
}

relationships.forEach((relationship, idx) => {
const otherEntityRelationshipName = relationships[idx].otherEntityRelationshipName;
const otherEntityRelationshipName =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mshima @atomfrede If we re-calculate this relationship name here, this can be an issue due to the size of the relationship length (we are adding more 4 characters HAL_)?

@DanielFran
Copy link
Member

@Evertude can you fix the conflicts?

@atomfrede did you have time to review the PR?

@atomfrede
Copy link
Member

Looks fine but didn't found time to test it.

@Evertude
Copy link
Contributor Author

Merged by using my version with removed findAllEager methods.

A note for the future: The query that you used wouldn't work as intended anyways because the MATCH statement requires an existing relation to another entity to return a result. The findAll wouldn't return entities that don't have relations

…into main

# Conflicts:
#	generators/entity-server/templates/src/main/java/package/repository/EntityRepository.java.ejs
@mraible
Copy link
Contributor

mraible commented Dec 29, 2020

@Evertude Please fix conflicts and let's merge this!

…into main

# Conflicts:
#	generators/entity-server/templates/src/main/java/package/domain/Entity.java.ejs
@Evertude
Copy link
Contributor Author

Evertude commented Jan 4, 2021

@DanielFran The maximum relationship length in Neo4J is 65k, I doubt that the 4 more characters make a difference: https://neo4j.com/docs/cypher-manual/current/syntax/naming/

@DanielFran
Copy link
Member

@Evertude trusting you on this.
@atomfrede Since the Neo4J e2e tests are broken for a while, I merge it.

@DanielFran DanielFran merged commit 18bd1bd into jhipster:main Jan 4, 2021
@Evertude
Copy link
Contributor Author

Evertude commented Jan 5, 2021

While working with M:N relationships, I realized that the OGM has pretty weird behavior when saving entities.
It sometimes doesn't add relations and even removes relations from other entities.

I read up a little and it seems like you are supposed to use RelationEntities for m:n relations.
That would require a sizeable re-implementation of the generator though.

For now I don't really have a solution and I don't know if its worse to randomly delete and not add stuff or to run in an infinite loop but at least my contribution should fix 1:N relations and M:N reads.

Please keep the Beta flag lol

@pascalgrimaud pascalgrimaud added this to the 7.0.0-beta.1 milestone Jan 16, 2021
sendilkumarn added a commit to jhipster/jhipster-kotlin that referenced this pull request Feb 27, 2021
sendilkumarn added a commit to jhipster/jhipster-kotlin that referenced this pull request Apr 18, 2021
sendilkumarn added a commit to jhipster/jhipster-kotlin that referenced this pull request Apr 18, 2021
* finished first cut for 7.0.0 release

* feat: upgrade the entity generator to v7 beta

* feat: upgrade server generator to v7

* feat: upgrader server generator to v7

* feat: update configuration to v7

* feat: migrate web/rest to v7

* feat: upgrade security to v7.0

* migrate repository tov v7 beta

* migrate service to v7 beta

* feat: upgrade config test directory to version 7

* migrate cucumber to v7

* feat: migrate gateway to v7

* feat: migrate repository test to v7

* feat: migrate security/jwt to v7

* feat: migrate cookie token extractor to v7

* feat: migrate security to v7

* feat: migrate mapper to v7

* feat: migrate service and test service to v7

* feat: migrate rest resources to v7

* feat: migrate Cassandra config test utils to v7

* fix: type configuration fixes

* feat: migrate test and test integration scripts to v7

* migrate workflows to v7 step 1

* migrate tests and integration scripts

* feat: migrate files to v7

* fix: use proper generic import

* feat: upgrade other files to v7

* feat: update expected-files to v7

* feat: update app spec to v7

* feat: test updates and remove import static

* add missing jhi_clone path

* fix: lint fixes

* fix ejs lint error

* fix: remove duplicate sonar analysis

* fix template lint issues

* feat: fix build issues

* fix: entity server unit test

* fix: failing test case because of missing prettier transform

* remove withAudit configuration

* feat: add postWriting in server configuration

* rename primaryKeyType to primaryKey.type

* rename JHipsterBlockHoundIntegration

* compilation fixes

* fix: some more bugs

* fix: add fun modifier for functions

* fix: remove extra braces and use proper field accessor

* fix: add a line break between package and import

* fix: add secondary constructor for the UserDTO

* fix: use proper variable declaration in patch_tempalte

* fix di and type selection

* fix: add variable name

* fix: migrate entity generator to v7 and few other fixes

* fix: make converters as an object

* fix: remove trailing commas

* fix: remove return type reference

* fix: use kotlin class reference

* fix: infer type

* fix build file issue

* fix: template string

* fix: mongodb kafka app generation

* fix: enity configuration and lint issues

* remove couchbase and fix kapt

* fix: react ci build issues

* lint fix

* fix generated services and entities

* fix react builds and remove couchbase builds

* make authorities proper

* Upgrade kotlin to latest version

* fix test failures in react default application

* lint fixes and failing unit test fixes

* fix: add a proper script folder

* fix: update cloned path for angular ci

* revert change

* fix more type issues

* Update EntityResourceIT.kt.ejs

* fix react ci tests

* Update PublicUserResource.kt.ejs

* webflux fixes

* feat/webflux fixes for v7 migration

* few more fixes

* fix cucumber tests

* replace proper cassandra version variable

* fix: workflow and kotlinify

* feat: add support for --pk-type

part of jhipster/generator-jhipster#13296

* Add support to `@MapstructExpression`

Refer jhipster/generator-jhipster#13195

* fix: Replace deprecated `StringUtils.isEmpty()` by `ObjectUtils.isEmpty()` and cover its usage with tests

Refer jhipster/generator-jhipster#13369

* chore: Change from 'idx in' to 'item of' for fields and relationships at server templates.

Refer jhipster/generator-jhipster#13382

* fix: Neo4j relation rework

Refer jhipster/generator-jhipster#13106

* fix: Swagger UI for reactive applications

Refer jhipster/generator-jhipster#13409

* fix: sonar code smells

Refer jhipster/generator-jhipster#13408

* chore: typo updates

* feat: Add custom claim converter in a microservice with oauth2

Refer jhipster/generator-jhipster#12609

* fix: import fixes for custom claim converter test

* fix: Fix reactive tests by assigning @NotNull a default message

Refer: jhipster/generator-jhipster#13483

* fix: R2DBC with OAuth

Refer jhipster/generator-jhipster#13485

* fix: Remove Transactional when using no db

Refer:  jhipster/generator-jhipster#13489

* fix: Ensure /websocket/tracker/ cant bypass

Refer: jhipster/generator-jhipster#13440

* fix: Fix R2DBC with OAuth

Refer: jhipster/generator-jhipster#13493

* chore: fix minor code smells

* fix: Broken Swagger with Microservice

Refer: jhipster/generator-jhipster#13446

* fix: minor updates

Refer: jhipster/generator-jhipster#13495

* fix: Change save template to use precedence

Refer: jhipster/generator-jhipster#12802:
jhipster/generator-jhipster#12802

* fix: add where clause from data jpa criteria when reactive

Refer: jhipster/generator-jhipster#13515

* fix: update dependencies and linting fixes

* fix: Update build pipeline versions

* fix: Angular workflow

* fix: copy error

* fix: missing otherEntityRelationshipNamePlural variable

* fix: ci script

* fix: remove unsupported hr language

* fix: more angular ci fixes

* fix: more angular ci fixes

* fix: move CustomClaimConverter to security package

* fix: expected files

* fix: ci to use installed version of jhipster

* rename webpack profile to webapp

* fix: refactor ci test

* fix: scripts path

* fix use proper path

* fix: add script level JHI_HOME

* fix: use correct  kotlin-samples folder

* chore: [ci] update chmod

* fix: missing variable and correct file location

* chore: [CI] add JHI_SCRIPTS location

* chore: [CI] add JHI_CLONED_SCRIPTS location

* chore: [CI] add HOME location

* chore: [CI] add HOME location

* Use tilde for home folder

* remove 13-replace-jhipster-version script

* temp: check scripts folder

* fix: use local scripts

* chore: [CI] add default work directory

* fix: use leading comma

* chore: [CI] remove additional app test

* minor fixes

* fix: use proper primary key name

* fix: replace id with primaryKey.type

* chore: [CI] add react workflow

* chore: [CI] upgrade angular workflow

* chore: [CI] add webflux workflow

* fix: entityResource

* chore: [CI] more workflow changes

* chore: [CI] Remove prettier check

* chore: [CI] fix react and webflux workflow

* fix: add UUID import for cassandra

* chore: [CI] use proper script for webflux

* fix: use apply instead of constructor params

* fix: change the constructor params

* fix: add UUIDFilter import for entity criteria

* fix: make queryservice to use properly typed filter

* fix: tests to have proper primarykey reference

* fix: use proper type for filter in EntityQueryService

* fix: type issues

* fix: redis configuration of test containers

* fix: generator entity spec test

* fix: entity post fix

* chore: [Test] Lint fixes

* chore: remove maven cucumber tests

* fix: sql webflux issues

* fix: use property accessor instead of getter

* chore: CI remove kafka test case

* fix: remove string utils

* Update EntityRepositoryInternalImpl_reactive.kt.ejs

* fix: add test fixes and other converters

* feat: migrate templates to v7

* lint fixes

* fix: update dependencies

* update package lock with npm v7

* fix: mocha tests

* lint fixes

* fix: double ejs tags

* fix: removed extra ejs tag

* fix: refactor missing type definition

* fix: use proper class variable

* fix: import repository always

* fix: clean up tests

* fix: use proper variable

* fix: remove mock builder standalone setup

* fix: reactive templates

* fix: remove reactive block

* fix: reactive modify server open api filter

* fix: reactive gateway

* fix: more reactive fixes

* fix: reactive file generation

* fix: no return in save template

* fix more reactive test

* fix: remove deprecated size configuration

* fix row mapper type checks

* fix: tests in custom claim converter

* fix: custom claim converter test

* fix: columnConverter for webflux r2dbc

* fix: extra parentheis

* fix: null checks

* fix: use empty Set instead of null

* fix: tests and persistence constructor

* fix: add coroutines lib for webflux

* fix: add kotlin extensions library

* fix: add reactor dependencies for maven

* fix: remove mariadb converter issue

* fix: webflux build and jdk version

* fix: generator unit test

* fix: install generator after npm ci

* fix: change reactive containers for psql

* fix: remove tc in the database url

* fix: remove reactive sql tests

* chore: remove azure pipelines
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 this pull request may close these issues.

None yet

7 participants