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

Add MySQL #320

Merged
merged 5 commits into from
Dec 14, 2021
Merged

Add MySQL #320

merged 5 commits into from
Dec 14, 2021

Conversation

swarajsaaj
Copy link
Contributor

Add support for MySQL database

Fix #231

I have replicated the postgresql style here without extracting common functionalities to avoid making a complicated if-else ladder in future.

@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

Merging #320 (4094acc) into main (65c1228) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                main      #320    +/-   ##
============================================
  Coverage     100.00%   100.00%            
- Complexity       463       493    +30     
============================================
  Files             80        85     +5     
  Lines           1479      1593   +114     
  Branches          32        32            
============================================
+ Hits            1479      1593   +114     
Impacted Files Coverage Δ
...ase/mysql/application/MySQLApplicationService.java 100.00% <100.00%> (ø)
...server/springboot/database/mysql/domain/MySQL.java 100.00% <100.00%> (ø)
...boot/database/mysql/domain/MySQLDomainService.java 100.00% <100.00%> (ø)
.../infrastructure/config/MySQLBeanConfiguration.java 100.00% <100.00%> (ø)
...sql/infrastructure/primary/rest/MySQLResource.java 100.00% <100.00%> (ø)

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 65c1228...4094acc. Read the comment docs.

@pascalgrimaud
Copy link
Member

Wow, thanks @swarajsaaj !
I'll review it soon

result.put("spring.datasource.url", "jdbc:mysql://localhost:3306/" + baseName);
result.put("spring.datasource.username", "root");
result.put("spring.datasource.password", "");
result.put("spring.datasource.driver-class-name", "com.mysql.jdbc.Driver");
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, these properties are not needed. Removed

result.put("spring.datasource.hikari.auto-commit", false);

result.put("spring.data.jpa.repositories.bootstrap-mode", "deferred");
result.put("spring.jpa.properties.hibernate.dialect", "org.hibernate.dialect.MySQL8Dialect");
Copy link
Member

Choose a reason for hiding this comment

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

same, I don't think we need this. What I can suggest, is to generate a JHipster project with MySQL and check all properties in application-prod.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, these properties are not needed. Removed

public class MySQL {

public static final String TESTCONTAINERS_VERSION = "1.16.0";

Copy link
Member

Choose a reason for hiding this comment

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

just an idea, maybe we can add: DOCKER_MYSQL_VERSION = "mysql:8.0.27"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes sure, I think constant name could be DOCKER_IMAGE_NAME instead?

Copy link
Member

Choose a reason for hiding this comment

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

yes it's ok :)

private Map<String, Object> springPropertiesForTest(String baseName) {
TreeMap<String, Object> result = new TreeMap<>();
result.put("spring.datasource.driver-class-name", "org.testcontainers.jdbc.ContainerDatabaseDriver");
result.put("spring.datasource.url", "jdbc:tc:mysql:8.0.27:///" + baseName);
Copy link
Member

Choose a reason for hiding this comment

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

we could use MySQL.getDockerMysqlVersion(), following my comment above

version: '3.8'
services:
mysql:
image: mysql:8.0.27
Copy link
Member

Choose a reason for hiding this comment

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

we could use {{dockerMysqlVersion}}, injected by DOCKER_MYSQL_VERSION value


@Override
public void addDockerCompose(Project project) {
project.addDefaultConfig(BASE_NAME);
Copy link
Member

Choose a reason for hiding this comment

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

we could add something like:

project.addConfig("dockerMysqlVersion", MySQL.getDockerMysqlVersion());


public static String getTestcontainersVersion() {
return TESTCONTAINERS_VERSION;
}
Copy link
Member

Choose a reason for hiding this comment

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

with new method getDockerMysqlVersion()

assertFileContent(
project,
getPath(TEST_RESOURCES, "config/application.properties"),
List.of("spring.datasource.url=jdbc:tc:mysql:8.0.27:///chips", "spring.datasource.username=chips")
Copy link
Member

Choose a reason for hiding this comment

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

use MySQL.getDockerMysqlVersion()

@pascalgrimaud
Copy link
Member

By reviewing this PR, I need to improve the code in PostgreSQL too :)

@pascalgrimaud pascalgrimaud merged commit a9f4783 into jhipster:main Dec 14, 2021
@pascalgrimaud
Copy link
Member

Don't forget to claim your bounty @swarajsaaj
Well deserved

@swarajsaaj
Copy link
Contributor Author

@pascalgrimaud
Copy link
Member

@swarajsaaj : approved!

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.

Spring Boot Database: MySQL
2 participants