Skip to content

Add initial dialect support #24

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

Closed
wants to merge 3 commits into from

Conversation

mp911de
Copy link
Member

@mp911de mp911de commented Nov 22, 2018

We now support dialects to integrate with various database vendors to support a similar feature set when seen from an application perspective.

We ship with dialects for Postgres, Microsoft SQL Server, and H2. We generate SQL statements through ReactiveDataAccessStrategy that considers dialects. Mapped entity support is limited to a single primary key, composite primary keys are not yet supported.

SQL generation happens on a basic level and is subject to further evolution.

Tests are now reduced to the minimal common guarantees.

@mp911de mp911de force-pushed the issue/gh-20-initial-dialect-support branch from edad8ca to 54baf40 Compare November 22, 2018 15:42
@mp911de mp911de requested a review from schauder November 27, 2018 08:05
Copy link
Contributor

@schauder schauder left a comment

Choose a reason for hiding this comment

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

I noticed you expect a Postgres database running. While feasible for a single vendor, this doesn't scale to multiple vendors, so I vote for using Testcontainers. I wonder if we could/want to create Testcontainers support that provides a ConnectionFactory directly.

This could be valuable for people experimenting with R2DBC.

package org.springframework.data.r2dbc.repository.config;
package org.springframework.data.r2dbc.config;

import io.r2dbc.spi.ConnectionFactory;
Copy link
Contributor

Choose a reason for hiding this comment

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

Someone has the wrong formatter configuration, which is interesting since you are supposed to be the only editor of the file.

* @throws IllegalArgumentException if any of the {@literal mappingContext} is {@literal null}.
*/
@Bean
public ReactiveDataAccessStrategy reactiveDataAccessStrategy(RelationalMappingContext mappingContext) {

Assert.notNull(mappingContext, "MappingContext must not be null!");
return new DefaultReactiveDataAccessStrategy(new BasicRelationalConverter(mappingContext));
return new DefaultReactiveDataAccessStrategy(getDialect(connectionFactory()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return new DefaultReactiveDataAccessStrategy(getDialect(connectionFactory()),
return new DefaultReactiveDataAccessStrategy(getDialect(connectionFactory()),

Copy link
Member Author

Choose a reason for hiding this comment

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

We typically want to share a single MappingContext across the configuration setup. With only a Dialect, DefaultReactiveDataAccessStrategy creates a converter and MappingContext on its own.

.database("foo").username("bar").password("password").build());
H2ConnectionFactory h2 = new H2ConnectionFactory(H2ConnectionConfiguration.builder().inMemory("mem").build());

assertThat(Database.findDatabase(postgres)).contains(Database.POSTGRES);
Copy link
Contributor

Choose a reason for hiding this comment

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

For these kind of independent asserts I like to use SoftAssertions

@mp911de
Copy link
Member Author

mp911de commented Dec 3, 2018

I filed #29 to add TestContainers support. It would be neat to have R2DBC in TestContainers (I think one might want to consume a container either via JDBC, R2DBC or both), paging @bsideup, what do you think?

mp911de and others added 3 commits December 3, 2018 15:38
…QL Server.

We now provide dialect support for H2, PostgreSQL, and Microsoft SQL Server databases, configurable through AbstractR2dbcConfiguration. By default, we obtain the Dialect by inspecting ConnectionFactoryMetadata to identify the database and the most likely dialect to use.

BindableOperation encapsulates statements/queries that can accept parameters. Use BindableOperation for statements through DatabaseClient.
Extract SQL creation.

Split integration test into abstract base class that can be implemented with a database-specific test class.
@mp911de mp911de force-pushed the issue/gh-20-initial-dialect-support branch from 1a2046e to 1f21d0d Compare December 3, 2018 14:42
mp911de added a commit that referenced this pull request Dec 3, 2018
…QL Server.

We now provide dialect support for H2, PostgreSQL, and Microsoft SQL Server databases, configurable through AbstractR2dbcConfiguration. By default, we obtain the Dialect by inspecting ConnectionFactoryMetadata to identify the database and the most likely dialect to use.

BindableOperation encapsulates statements/queries that can accept parameters. Use BindableOperation for statements through DatabaseClient.
Extract SQL creation.

Split integration test into abstract base class that can be implemented with a database-specific test class.

Original pull request: #24.
mp911de pushed a commit that referenced this pull request Dec 3, 2018
Rename Database.latestDialect() to defaultDialect(). Rename Dialect.returnGeneratedKeys() to Dialect.generatedKeysClause(). Simplify IndexBindMarkers. Create BindableOperation.bind(Statement, SettableValue) to reduce code duplicates. Rename BindSpecWrapper to BindSpecAdapter.

Original pull request: #24.
@mp911de
Copy link
Member Author

mp911de commented Dec 3, 2018

That's merged and polished now.

@mp911de mp911de closed this Dec 3, 2018
@mp911de mp911de deleted the issue/gh-20-initial-dialect-support branch December 3, 2018 15:03
@bsideup
Copy link

bsideup commented Dec 3, 2018

@mp911de great idea! Should be super easy to add 👍

@mp911de
Copy link
Member Author

mp911de commented Dec 3, 2018

Do you want me to file a ticket?

@bsideup
Copy link

bsideup commented Dec 3, 2018

Yes, please 😊

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.

3 participants