jhipster / generator-jhipster

JHipster is a development platform to quickly generate, develop, & deploy modern web applications & microservice architectures.
https://www.jhipster.tech
Apache License 2.0
21.51k stars 4.02k forks source link

Couchbase tests are failing #13544

Closed mraible closed 2 years ago

mraible commented 3 years ago
Overview of the issue

Failure seems to be caused by EJS issues.

WARNING! Copying template /home/runner/generator-jhipster/generators/entity-server/templates/src/main/java/package/domain/Entity.java.ejs failed. [TypeError: /home/runner/generator-jhipster/generators/entity-server/templates/src/main/java/package/domain/Entity.java.ejs:680
    678|         this.set<%= relationshipNameCapitalized %>(<%= otherEntityName %>);
    679| <%_ if ((databaseType === 'couchbase' && !otherEntityIsEmbedded) || reactiveRelationshipWithId) { _%>
 >> 680|         this.<%= relationshipFieldName %>Id = <%= otherEntityName %> != null ? <%= otherEntityName %>.get<%= primaryKey.nameCapitalized %>() : null;
    681| <%_ } _%>
    682|         return this;
    683|     }

Cannot read property 'nameCapitalized' of undefined]
ERROR! /home/runner/generator-jhipster/generators/entity-server/templates/src/main/java/package/domain/Entity.java.ejs:680
    678|         this.set<%= relationshipNameCapitalized %>(<%= otherEntityName %>);
    679| <%_ if ((databaseType === 'couchbase' && !otherEntityIsEmbedded) || reactiveRelationshipWithId) { _%>
 >> 680|         this.<%= relationshipFieldName %>Id = <%= otherEntityName %> != null ? <%= otherEntityName %>.get<%= primaryKey.nameCapitalized %>() : null;
    681| <%_ } _%>
    682|         return this;
    683|     }

Cannot read property 'nameCapitalized' of undefined
TypeError: /home/runner/generator-jhipster/generators/entity-server/templates/src/main/java/package/domain/Entity.java.ejs:680
    678|         this.set<%= relationshipNameCapitalized %>(<%= otherEntityName %>);
    679| <%_ if ((databaseType === 'couchbase' && !otherEntityIsEmbedded) || reactiveRelationshipWithId) { _%>
 >> 680|         this.<%= relationshipFieldName %>Id = <%= otherEntityName %> != null ? <%= otherEntityName %>.get<%= primaryKey.nameCapitalized %>() : null;
    681| <%_ } _%>
    682|         return this;
    683|     }

Cannot read property 'nameCapitalized' of undefined
    at module.exports.eval (/home/runner/generator-jhipster/generators/entity-server/templates/src/main/java/package/domain/Entity.java.ejs:1460:37)
    at Entity.java (/home/runner/generator-jhipster/node_modules/ejs/lib/ejs.js:691:17)
    at tryHandleCache (/home/runner/generator-jhipster/node_modules/ejs/lib/ejs.js:272:36)
    at Object.exports.renderFile (/home/runner/generator-jhipster/node_modules/ejs/lib/ejs.js:489:10)
    at Object.renderContent (/home/runner/generator-jhipster/generators/utils.js:250:9)
    at module.exports.template (/home/runner/generator-jhipster/generators/generator-base-private.js:755:23)
    at /home/runner/generator-jhipster/generators/generator-base.js:2233:42
    at Array.forEach (<anonymous>)
    at /home/runner/generator-jhipster/generators/generator-base.js:2149:45
    at Array.forEach (<anonymous>) {
  path: '/home/runner/generator-jhipster/generators/entity-server/templates/src/main/java/package/domain/Entity.java.ejs'
}
Error: Process completed with exit code 1.
Motivation for or Use Case

All our nightly builds on https://github.com/hipster-labs/jhipster-daily-builds should pass.

pascalgrimaud commented 3 years ago

Nothing new here, @mraible Couchbase is failing since months. See https://github.com/hipster-labs/jhipster-daily-builds/actions?query=workflow%3ACouchbase

See this ticket too: https://github.com/jhipster/generator-jhipster/issues/12300

mraible commented 3 years ago

It's now a migration issue. We need to upgrade to Spring Data Couchbase 4.x. https://docs.spring.io/spring-data/couchbase/docs/4.1.3/reference/html/#couchbase.migrating

mraible commented 3 years ago

If you have issues with contributing to my pull request, please let me know. I'd be happy to make it part of JHipster's repo instead of my fork.

mshima commented 3 years ago

There is another unfinished pr from murdos https://github.com/jhipster/generator-jhipster/pull/11845

mraible commented 3 years ago

Thanks for that @mshima! I integrated @murdos' changes into https://github.com/jhipster/generator-jhipster/pull/13770. I'm able to create the ngx-couchbase app with entities and all e2e tests pass. Tests are failing, not sure why.

mmonti commented 3 years ago

Hi! I have been looking into the failing test and I think I found at least a couple problems that seem to be the root cause of these failing tests.

The first one is that the default scan consistency in Spring Data Repository is set to NOT_BOUNDED. This mean that for every write we sent to CB, the response is immediate (we don't wait for the documents to be indexed). If we issue a read immediately after we get inconsistent results. That is pretty much the behavior I saw running the tests. For some tests, doing some line by line debugging, the tests passed since I introduced some wait during the execution of each statement.

The other issue I found seems to be related with how Spring Data generates the queries from the repositories.in
As an example, one of the tests i checked is testFinishPasswordReset(). This test stores a document in Couchbase with the following structure:

{
  "last_modified_date": 1612115098027,
  "reset_key": "reset key",
  "last_modified_by": "test",
  "login": "finish-password-reset",
  "created_by": "test",
  "authorities": [],
  "password": "晛漦𒓻𨰁𬐲𬱜𤑖̗̎𦱼腀ᴴ믑미𥲗𢕉𠶃𦴊䶙䋹𑙪艓𫿜𥆠𮓛轢𦲉𨪳𓏤𣜃𝓑㼀𬭥鹎썣촣䙖𧾙㺌",
  "reset_date": 1612115157966,
  "_class": "com.mycompany.myapp.domain.User",
  "created_date": 1612115098027,
  "email": "finish-password-reset@example.com",
  "activated": false
}

and then it makes a request to fetch the document by "reset_key". The method to fetch is defined in the UserRepository.java as:

Optional<User> findOneByResetKey(String resetKey);

Now, the statement executed in Couchbase is:

SELECT META(`testBucket`).id AS __id, META(`testBucket`).cas AS __cas, `testBucket`.* FROM `testBucket` WHERE `_class` = "com.mycompany.myapp.domain.User" AND `resetKey` = $1

Notice that the attribute is "resetKey" and not "reset_key" which cause the method to return no results and fail the test. The same happen with other tests that fetch for "activationKey". It seems to be a bug in Spring Data since the User entity it has a "Field" annotation with a value and Spring Data should use that value to generate the correct query.

One of the things we can try is to get the latest spring data couchbase version and see if it is fixed there. I am not very familiar with the Jhipster internals and need to do some more digging to check where the dependency versions are defined to bring the lastes or just modify the pom file and force the last version there. Ill give it a try tonight.

mraible commented 3 years ago

@mmonti You are correct in your scan consistency theory. I've been adding the annotation you mention on all the find*() methods. It's a real pain and only seems to be needed for tests. I asked about setting a global default on this closed issue in Spring Data Couchbase.

Your other finding about key names is interesting.

We're currently using org.springframework.data:spring-data-couchbase:jar:4.1.3 in JHipster. We could try upgrading to 4.2.0-M2 or 4.2.0-SNAPSHOT, but I'm not sure that'll fix things.

mraible commented 3 years ago

@mmonti I created a couple of repos from my branch so you can see the test failures.

If you run ./mvnw verify after cloning them, you'll see the integration test failures.

You can also run them and see that e2e tests pass.

npm install
docker-compose -f src/main/docker/couchbase.yml up -d
./mvnw
# open a new terminal after it finishes starting
npm run e2e
mmonti commented 3 years ago

Hi @mraible, thanks for the repos! TLDR; all tests passed... :)

I mainly worked on a fork of https://github.com/mraible/ngx-couchbase and focus again on the failing integration tests. I found the root cause of the issue I mentioned earlier (the one with spring data generating the wrong query for fields with @Field annotations).

The fix seems to be pretty simple in Spring Data (N1qlQueryCreator.java).

In that converter of using getName() on source we need to change that to getFieldName() which has the logic to check for @Field annotation and obtain the value from it in case it is present or fallback to getName() through fieldNameStrategy if not.

I extended the Spring Data Couchbase Repository infrastructure to test this, and fixed a couple more tests of the 6 remaining. I am planning on creating a PR in your repo so you can take a look and give it a try. I will also try to look into Spring Data project to see if that issue is already reported and if not try to contribute that fix.

Now, there were a couple more Integration tests that failed after my fix and are related with other "new issues" that I discovered. One is related with how Spring Data generates the query for this respository method:

@ScanConsistency(query = QueryScanConsistency.REQUEST_PLUS)
List<User> findAllByIdNotNullAndActivatedIsTrue();

This method generates:

SELECT META(`testBucket`).id AS __id, META(`testBucket`).cas AS __cas, `testBucket`.* FROM `testBucket` WHERE `_class` = "tech.jhipster.sample.domain.User" AND id is not null and `activated`

which seems OK and straightforward for any DB but in Couchbase, IDs are treated in a different way and they need to be accessed through the metadata object. So the query that method should generate, should be something like:

SELECT META(`testBucket`).id AS __id, META(`testBucket`).cas AS __cas, `testBucket`.* FROM `testBucket` WHERE `_class` = "tech.jhipster.sample.domain.User" AND meta(`testBucket`).id is not null and `activated`

I did succeed fixing this in Spring Data N1qlQueryCreator (in the same Converter lambda mentioned earlier) but the fix is more involved since we also need to tweak the QueryCriteria class (there is some escaping with backticks in that class that needs to be handled). I will try to report this to the Spring Data team as well, although I am thinking that the meta().id not being supported in derived query methods is by design.

This is because they provide the @Query annotation and we can just annotate that method and add the behavior we want. Of course this needs to be changed in the JHipster Generator. Anyways, It would be nice to double check with them.

The other test that fails is:

@Test
void assertThatUserCanBeFoundByEmailIgnoreCase() {
    UserDetails userDetails = domainUserDetailsService.loadUserByUsername(USER_TWO_EMAIL.toUpperCase(Locale.ENGLISH));
    assertThat(userDetails).isNotNull();
    assertThat(userDetails.getUsername()).isEqualTo(USER_TWO_LOGIN);
}

and this one is also related with a repository method, but in this case I believe that is because the Couchbase implementation does not support the "IgnoreCase" in:

@ScanConsistency(query = QueryScanConsistency.REQUEST_PLUS)
Optional<User> findOneByEmailIgnoreCase(String email);

This method generates the follow query:

SELECT META(`testBucket`).id AS __id, META(`testBucket`).cas AS __cas, `testBucket`.* FROM `testBucket` WHERE `_class` = "tech.jhipster.sample.domain.User" AND `email` = $1

and in Couchbase we should have something like:

SELECT META(`testBucket`).id AS __id, META(`testBucket`).cas AS __cas, `testBucket`.* FROM `testBucket` WHERE `_class` = "tech.jhipster.sample.domain.User" AND lower(`email`) = lower($1)

I think that we can probably solve this test changing the JHipster Generator and change the method name if the database is couchbase or just use a @Query annotation directly in the method. I actually fixed it in my fork with @Query:

@Query("#{#n1ql.selectEntity} WHERE LOWER(email) = LOWER($1) AND #{#n1ql.filter}")

With all these changes in ngx-couchbase, all tests passed and we should pretty good to retry the build. Anyways, I still need to check if these fixes are valid also for the reactive version. Will try to look into during this week.

mikereiche commented 3 years ago

https://github.com/spring-projects/spring-data-couchbase/issues/1055

mmonti commented 3 years ago

Ohh dang!... I feel that i did all this debugging for issues that were already addressed!... 🤦‍♂️ 😂 I should have browse the PRs/Issues first in Spring Data Couchbase.

Anyways, I am glad that DATACOUCH-1055 is already merged. I think that with that fix in, we can work out the rest on JHipster side.

mraible commented 3 years ago

@mmonti I updated the webflux-couchbase example after incorporating your fixes. There are some tests failing. See details at https://github.com/jhipster/generator-jhipster/pull/13770#issuecomment-773615644.

mmonti commented 3 years ago

@mraible thanks!... I started to look into the reactive implementation but didn't make much progress yet. I will try to work on it in the next few days.

mikereiche commented 3 years ago

I see several test failures related to pagination. The reactive pagination support is... unstable.

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 30 days with no activity. Our core developers tend to be more verbose on denying. If there is no negative comment, possibly this feature will be accepted. We are accepting PRs :smiley:. Comment or this will be closed in 7 days

mraible commented 3 years ago

I'm hopeful we'll be able to fix these issues for v7. If not, there's this PR to disable the prompt for Couchbase. It will still be an option if you use JDL.

mikereiche commented 3 years ago

"The first one is that the default scan consistency in Spring Data Repository is set to NOT_BOUNDED. " https://github.com/spring-projects/spring-data-couchbase/issues/1062 is a feature request to support a repository-level ScanConsistency.

mikereiche commented 3 years ago

"It seems to be a bug in Spring Data since the User entity it has a "Field" annotation with a value and Spring Data should use that value to generate the correct query."

This is https://github.com/spring-projects/spring-data-couchbase/issues/1055

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 30 days with no activity. Our core developers tend to be more verbose on denying. If there is no negative comment, possibly this feature will be accepted. We are accepting PRs :smiley:. Comment or this will be closed in 7 days

mikereiche commented 2 years ago

Hi @pascalgrimaud - I see this has been reopened - but it looks like the couchbase tests are passing. Please let me know if there is any other information. https://github.com/hipster-labs/jhipster-daily-builds

pascalgrimaud commented 2 years ago

Yes, I reopen stale tickets, as they have been auto closed As it's solved, I can close it. Thanks :-)