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 SDK 3, Spring Data Couchbase 4 Integration #15645

Merged
merged 128 commits into from Aug 26, 2021
Merged

Conversation

gkysaad
Copy link
Contributor

@gkysaad gkysaad commented Jul 14, 2021

This PR implements a fix for and is related to the following Issues/PRs: #14184, #11845, #13770, #13544, #12300

This adds support for this issue: #14745 and was tested with the latest stable version of Couchbase (6.6.2) and uses Spring Data Couchbase 4 (Couchbase Java SDK 3).

I have used @mraible's fix-couchbase branch here and in PR #14184 as a starting point for my work on this.

Some notes:

  • I have disabled a couple of the integration tests as they were flaky (but worked fine when the endpoint was tested locally). These tests are: UserResourceIT.getAllUsers() and PublicUserResourceIT.getAllAuthorities().
  • Some of the other tests are flaky as well, but pass the majority of the time (and are random), so rerunning the pipeline usually makes it pass. I can disable these as well if I pinpoint any that fail more frequently.
  • This fix is for the non-reactive Couchbase implementation, so Vue, React, and Angular tests were re-enabled, but not Webflux
  • @N1qlJoin support in Spring Data Couchbase 4 seems to be broken or disabled - there is little documentation or info about it online as well, so I've switched over the way document relationships are handled to use the normal @Field annotations to embed the documents. I will create a separate issue in case anyone would like to look into this further.

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 Jul 14, 2021

CLA assistant check
All committers have signed the CLA.

@DanielFran
Copy link
Member

@gkysaad Can you please sign the cla?

@Tcharl
Copy link
Contributor

Tcharl commented Jul 20, 2021

Whow, it looks like there's a lot of code to achieve in order to consume couchbase with spring

@DanielFran
Copy link
Member

@gkysaad Can you fix the conflicts please?

@gkysaad
Copy link
Contributor Author

gkysaad commented Aug 5, 2021

Apologies for the delay on the conflicts/CLA - should be able to finish that up soon

@tchlyah
Copy link
Contributor

tchlyah commented Aug 8, 2021

Hello,

I tried to generate an app to test this, but I keep getting this error after generating some files, so the app is not fully generated.

ERROR! Callback called multiple times
Error [ERR_MULTIPLE_CALLBACK]: Callback called multiple times
    at NodeError (node:internal/errors:371:5)
    at onFinish (node:internal/streams/writable:667:37)
    at processTicksAndRejections (node:internal/process/task_queues:82:21)
    at runNextTicks (node:internal/process/task_queues:65:3)
    at listOnTimeout (node:internal/timers:526:9)
    at processTimers (node:internal/timers:500:7) {
  code: 'ERR_MULTIPLE_CALLBACK'
}

any one has an idea ?

@tchlyah
Copy link
Contributor

tchlyah commented Aug 10, 2021

Hello,

I tried to generate an app to test this, but I keep getting this error after generating some files, so the app is not fully generated.

ERROR! Callback called multiple times
Error [ERR_MULTIPLE_CALLBACK]: Callback called multiple times
    at NodeError (node:internal/errors:371:5)
    at onFinish (node:internal/streams/writable:667:37)
    at processTicksAndRejections (node:internal/process/task_queues:82:21)
    at runNextTicks (node:internal/process/task_queues:65:3)
    at listOnTimeout (node:internal/timers:526:9)
    at processTimers (node:internal/timers:500:7) {
  code: 'ERR_MULTIPLE_CALLBACK'
}

any one has an idea ?

Nevermind, the issue was about using a non LTS version of node, so I installed back the latest LTS version (14.17.4), and now it is working.

@mraible
Copy link
Contributor

mraible commented Aug 12, 2021

@gkysaad Can you please resolve conflicts?

@gkysaad
Copy link
Contributor Author

gkysaad commented Aug 13, 2021

Signed the CLA, working on the rebase

Copy link
Contributor

@tchlyah tchlyah left a comment

Choose a reason for hiding this comment

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

This seems to be a huge work, thanks for your help. Can you check out my comment please?

generators/server/templates/build.gradle.ejs Outdated Show resolved Hide resolved
generators/server/templates/build.gradle.ejs Outdated Show resolved Hide resolved
import org.springframework.data.repository.core.NamedQueries;
import <%= packageName %>.config.couchbase.CustomN1qlRepositoryQueryExecutor;

public class CustomCouchbaseRepositoryQuery extends CouchbaseRepositoryQuery {
Copy link
Contributor

Choose a reason for hiding this comment

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

CouchbaseRepositoryQuery is deprecated. Can you please explain exactly what the problem is that forces you to implement all this custom classes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already there on the branch that I started working from, so I'm not exactly sure. However, I didn't want to remove it so as to not break anything that depended on it. If you have any suggestions for removing or changing it, I can make those changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

After testing without it, it seems that there is some broken tests in AccountResourceIT due to this error:

java.lang.IllegalArgumentException: Authentication object cannot be null

I think we can find a more concise solution for that. But leave it for now. I'll try to look at it ASAP.

Added Couchbase tests to Github Actions and ensured they work with latest changes in the code
Only the non-reactive implementation of Couchbase was upgraded to the latest compatible SDK version
gkysaad and others added 4 commits August 22, 2021 23:27
…serRepository.java.ejs

Co-authored-by: Marcelo Shima <marceloshima@gmail.com>
…serRepository.java.ejs

Co-authored-by: Marcelo Shima <marceloshima@gmail.com>
@mshima
Copy link
Member

mshima commented Aug 23, 2021

561e5f3 is conflicting with 228719d

@gkysaad
Copy link
Contributor Author

gkysaad commented Aug 25, 2021

561e5f3 is conflicting with 228719d

It seems like there aren't any common files between those commits and everything looks fine on my end. The 561e5f3 commit was made to revert back to snake_case for the user initialization and the 228719d commit changes all the Couchbase code for the entities and users to use snake_case as well.

Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

There are 3 missing tasks:

Or we can disable the tests for now and merge as it is, so work can continue in main repository.

@gkysaad
Copy link
Contributor Author

gkysaad commented Aug 26, 2021

There are 3 missing tasks:

Or we can disable the tests for now and merge as it is, so work can continue in main repository.

I've completed the 2 tasks for those comments. As for the tests, when I made the PR, all the tests were passing and the functionality seemed to be working. There was some flakiness with some of the tests where they would not pass once but then pass when rerun. I disabled 2 that were consistently failing, but had working functionality. I think we should merge it as it is for now, then make a GitHub issue after the merge to look into the test flakiness.

@mshima
Copy link
Member

mshima commented Aug 26, 2021

I will cleanup the PR to merge.

@mshima
Copy link
Member

mshima commented Aug 26, 2021

Merging.

  • couchbase isn’t fully functional yet
  • current main is completely broken
  • improves modularization
  • conflicting too often

@mshima mshima merged commit 3a2e16c into jhipster:main Aug 26, 2021
@pascalgrimaud pascalgrimaud added this to the 7.2.0 milestone Sep 11, 2021
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

8 participants