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

Couchbase tests are failing #13544

Closed
mraible opened this issue Jan 15, 2021 · 21 comments
Closed

Couchbase tests are failing #13544

mraible opened this issue Jan 15, 2021 · 21 comments
Labels
area: bug 🐛 $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ theme: couchbase $300 https://www.jhipster.tech/bug-bounties/
Milestone

Comments

@mraible
Copy link
Contributor

mraible commented Jan 15, 2021

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
Copy link
Member

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: #12300

@mraible mraible added $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ $300 https://www.jhipster.tech/bug-bounties/ labels Jan 30, 2021
@mraible
Copy link
Contributor Author

mraible commented Jan 30, 2021

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
Copy link
Contributor Author

mraible commented Jan 30, 2021

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
Copy link
Member

mshima commented Jan 30, 2021

There is another unfinished pr from murdos #11845

@mraible
Copy link
Contributor Author

mraible commented Jan 30, 2021

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

@mmonti
Copy link

mmonti commented Jan 31, 2021

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.

  • One of the possible solutions is to mark each finder method with @ScanConsistency(query = QueryScanConsistency.REQUEST_PLUS). I couldnt really find a way to set a global consistency instead of doing it method by method.
  • Another option is to introduce some wait between read/writes in the test ¯_(ツ)_/¯

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
Copy link
Contributor Author

mraible commented Jan 31, 2021

@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
Copy link
Contributor Author

mraible commented Jan 31, 2021

@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
Copy link

mmonti commented Feb 1, 2021

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
Copy link

@mmonti
Copy link

mmonti commented Feb 1, 2021

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
Copy link
Contributor Author

mraible commented Feb 4, 2021

@mmonti I updated the webflux-couchbase example after incorporating your fixes. There are some tests failing. See details at #13770 (comment).

@mmonti
Copy link

mmonti commented Feb 4, 2021

@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
Copy link

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

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2021

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 😃.
Comment or this will be closed in 7 days

@mraible
Copy link
Contributor Author

mraible commented Mar 7, 2021

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
Copy link

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

@mikereiche
Copy link

"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 spring-projects/spring-data-couchbase#1055

@github-actions
Copy link
Contributor

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 😃.
Comment or this will be closed in 7 days

@pascalgrimaud pascalgrimaud added this to the 7.1.0 milestone May 28, 2021
gkysaad added a commit to gkysaad/generator-jhipster that referenced this issue Jul 14, 2021
gkysaad added a commit to gkysaad/generator-jhipster that referenced this issue Aug 13, 2021
@pascalgrimaud pascalgrimaud reopened this Jan 11, 2022
@mikereiche
Copy link

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: bug 🐛 $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ theme: couchbase $300 https://www.jhipster.tech/bug-bounties/
Projects
None yet
5 participants