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

Fix #691 - Add Mongock support #1424

Merged
merged 13 commits into from Apr 27, 2022
Merged

Conversation

beatfreaker
Copy link
Contributor

Fix #691 - Add Mongock support

@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #1424 (ee924e0) into main (3c7da02) will not change coverage.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##                main     #1424    +/-   ##
============================================
  Coverage     100.00%   100.00%            
- Complexity      1597      1612    +15     
============================================
  Files            305       317    +12     
  Lines           5227      5340   +113     
  Branches         106       106            
============================================
+ Hits            5227      5340   +113     
Impacted Files Coverage Δ
...mongock/application/MongockApplicationService.java 100.00% <100.00%> (ø)
...migration/mongock/domain/MongockDomainService.java 100.00% <100.00%> (ø)
...nfrastructure/config/MongockBeanConfiguration.java 100.00% <100.00%> (ø)
...k/infrastructure/primary/rest/MongockResource.java 100.00% <100.00%> (ø)
...bapp/app/springboot/primary/Generator.component.ts 100.00% <100.00%> (ø)
...rimary/react-generator/ReactGenerator.component.ts 100.00% <100.00%> (ø)
...pp/app/springboot/primary/react-generator/index.ts 100.00% <100.00%> (ø)
...ng-boot-generator/SpringBootGenerator.component.ts 100.00% <100.00%> (ø)
.../springboot/primary/spring-boot-generator/index.ts 100.00% <100.00%> (ø)
...mary/svelte-generator/SvelteGenerator.component.ts 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3af60a5...ee924e0. Read the comment docs.

@beatfreaker
Copy link
Contributor Author

@pascalgrimaud can you please review this PR

@pascalgrimaud
Copy link
Member

@beatfreaker : yes, will do it when possible. I'm a little bit late, as I have several PRs to review before this one. Just be patient :)

@pascalgrimaud
Copy link
Member

@beatfreaker : can you resolve the conflict plz?

# Conflicts:
#	src/main/resources/generator/dependencies/pom.xml

@Override
public void addConfigurationJava(Project project) {
String packageNamePath = project.getPackageNamePath().orElse(getPath("com/mycompany/myapp"));
Copy link
Member

Choose a reason for hiding this comment

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

use constant DefaultConfig for com/mycompany/myapp

@Override
public void addConfigurationJava(Project project) {
String packageNamePath = project.getPackageNamePath().orElse(getPath("com/mycompany/myapp"));
String mongockConfigPath = "technical/infrastructure/secondary/mongock";
Copy link
Member

Choose a reason for hiding this comment

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

you can use TECHNICAL_INFRASTRUCTURE_SECONDARY + mongock

@Override
public void addChangelogJava(Project project) {
project.addDefaultConfig(PACKAGE_NAME);
String packageNamePath = project.getPackageNamePath().orElse(getPath("com/mycompany/myapp"));
Copy link
Member

Choose a reason for hiding this comment

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

use constants in DefaultConfig

public void addChangelogJava(Project project) {
project.addDefaultConfig(PACKAGE_NAME);
String packageNamePath = project.getPackageNamePath().orElse(getPath("com/mycompany/myapp"));
String mongockDBMigrationPath = "technical/infrastructure/secondary/mongock/dbmigration";
Copy link
Member

Choose a reason for hiding this comment

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

use TECHNICAL_INFRASTRUCTURE_SECONDARY

Comment on lines 20 to 22
public void rollback() {}

}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public void rollback() {}
}
public void rollback() {}
}

Comment on lines 12 to 14
public InitialMigrationSetup(MongoTemplate template) {
this.template = template;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public InitialMigrationSetup(MongoTemplate template) {
this.template = template;
}
public InitialMigrationSetup(MongoTemplate template) {
this.template = template;
}

Comment on lines 38 to 43
ProjectDTO projectDTO = TestUtils.readFileToObject("json/chips.json", ProjectDTO.class);
if (projectDTO == null) {
throw new GeneratorException("Error when reading file");
}
projectDTO.folder(FileUtils.tmpDirForTest());
Project project = ProjectDTO.toProject(projectDTO);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ProjectDTO projectDTO = TestUtils.readFileToObject("json/chips.json", ProjectDTO.class);
if (projectDTO == null) {
throw new GeneratorException("Error when reading file");
}
projectDTO.folder(FileUtils.tmpDirForTest());
Project project = ProjectDTO.toProject(projectDTO);
ProjectDTO projectDTO = TestUtils.readFileToObject("json/chips.json", ProjectDTO.class).folder(tmpDirForTest());
Project project = ProjectDTO.toProject(projectDTO);

@pascalgrimaud
Copy link
Member

Very good job here, @beatfreaker
I still have 3 other comments:

  • plz add a package-info.java, defined as a BusinessContext
  • plz add a new README.md (similar to what is done in maven/README.md)
  • call the API in the generate.sh file, at the same level than mongodbapp

@beatfreaker
Copy link
Contributor Author

Hello @pascalgrimaud

currently the build is failing for mongodbapp execution, and I suspect it is because of following warning

Warning:  Rule violated for class tech.jhipster.fullapp.technical.infrastructure.secondary.mongock.dbmigration.InitialMigrationSetup: lines covered ratio is 0.00, but expected minimum is 1.00
Warning:  Rule violated for class tech.jhipster.fullapp.technical.infrastructure.secondary.mongock.MongockDatabaseConfiguration: lines covered ratio is 0.00, but expected minimum is 1.00

As these generated files does not have any logic so I don't know what should we write in unit tests, could you please help me with it ?

@pascalgrimaud
Copy link
Member

@beatfreaker : sure, what do you prefer? Suggestion in comment or commit directly to your branch ?

@beatfreaker
Copy link
Contributor Author

@pascalgrimaud I am okay with both,

If you have limited time then just add it here, I can put it in code and test it.

@@ -0,0 +1,21 @@
package tech.jhipster.fullapp.technical.infrastructure.secondary.mongock.dbmigration;
Copy link
Member

Choose a reason for hiding this comment

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

bad package here, you should use packageName

@@ -0,0 +1,8 @@
package tech.jhipster.fullapp.technical.infrastructure.secondary.mongock;
Copy link
Member

Choose a reason for hiding this comment

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

bad package here

@pascalgrimaud
Copy link
Member

@beatfreaker : here the simple unit test you can use:

@UnitTest
@ExtendWith(MockitoExtension.class)
class InitialMigrationSetupTest {

  @InjectMocks
  InitialMigrationSetup initialMigrationSetup;

  @Test
  void shouldChangeSet() {
    assertThatCode(() -> initialMigrationSetup.changeSet()).doesNotThrowAnyException();
  }

  @Test
  void shouldRollback() {
    assertThatCode(() -> initialMigrationSetup.rollback()).doesNotThrowAnyException();
  }
}

@beatfreaker
Copy link
Contributor Author

@beatfreaker : here the simple unit test you can use:

@UnitTest
@ExtendWith(MockitoExtension.class)
class InitialMigrationSetupTest {

  @InjectMocks
  InitialMigrationSetup initialMigrationSetup;

  @Test
  void shouldChangeSet() {
    assertThatCode(() -> initialMigrationSetup.changeSet()).doesNotThrowAnyException();
  }

  @Test
  void shouldRollback() {
    assertThatCode(() -> initialMigrationSetup.rollback()).doesNotThrowAnyException();
  }
}

This helps, thank you

@beatfreaker
Copy link
Contributor Author

beatfreaker commented Apr 26, 2022

@pascalgrimaud
sonar is reporting below code smell, I added it so that it will be helpful for user to start using MongoTemplate
is it fine leave it as commented, or we should totally remove it ?

Screenshot 2022-04-26 at 5 33 50 PM

@pascalgrimaud
Copy link
Member

Fine to remove it, in this initial migration file.
In the other migration files (for User for example), it will be there, so no worry about that


## Maintainers

- [Pascal Grimaud](https://github.com/pascalgrimaud)
Copy link
Member

Choose a reason for hiding this comment

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

Don't you want to take the lead on this part @beatfreaker ? :-)

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 can, updating it :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes plz put your name :-)

@pascalgrimaud pascalgrimaud merged commit 7e84164 into jhipster:main Apr 27, 2022
@pascalgrimaud
Copy link
Member

Thanks for your work here @beatfreaker ❤️

@beatfreaker
Copy link
Contributor Author

@pascalgrimaud
Copy link
Member

@beatfreaker : approved. If possible, can you contribute to the front part too? Otherwise, the feature won't be available

@beatfreaker
Copy link
Contributor Author

@pascalgrimaud yeah sure I can make respective changes on frontend too, will create new ticket to handle it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spring Boot Database Migration: Mongock support
2 participants