DATAJDBC-107 - Introduce naming strategy #12
Conversation
| @schauder if you wish to look over what I have coded and especially inspect the test cases that use this mechanism. |
gregturn force-pushed the issue/DATAJDBC-107 branch 2 times, most recently from 6af7316 to 152bf52 Compare 
| .contains("ref.l1id AS ref_l1id") // | ||
| .contains("ref.content AS ref_content").contains(" FROM DummyEntity"); | ||
| new SoftAssertions().assertAll(); | ||
| softAssertions.assertAll(); |
| @Test // DATAJDBC-104 | ||
| public void enumGetsStoredAsString() { | ||
| JdbcPersistentEntity<?> persistentEntity = new JdbcMappingContext().getRequiredPersistentEntity(DummyEntity.class); | ||
| JdbcPersistentEntity<?> persistentEntity = new JdbcMappingContext(new DefaultNamingStrategy()).getRequiredPersistentEntity(DummyEntity.class); |
There was a problem hiding this comment.
Empty line before and the line should be broken into two.
| import org.assertj.core.api.SoftAssertions; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
| |
There was a problem hiding this comment.
My formatting setup removes this empty line. I'm not sure which configuration is the correct one, i.e. matches the one of Oliver.
There was a problem hiding this comment.
Let's look in Spring Data JPA to divine an answer.
There was a problem hiding this comment.
Per Spring Data JPA line should be removed.
| }; | ||
| | ||
| final NamingStrategy upperCaseLowerCaseStrategy = new DefaultNamingStrategy() { | ||
| @Override |
| public class FixedNamingStrategyUnitTests { | ||
| | ||
| final NamingStrategy fixedCustomTablePrefixStrategy = new DefaultNamingStrategy() { | ||
| @Override |
There was a problem hiding this comment.
Blank line, right?
Also between the other methods.
There was a problem hiding this comment.
I'll double check. Don't code anonymous inner classes often in Spring Data.
| * demonstrating how to inject one while not breaking existing SQL operations. | ||
| */ | ||
| @Bean | ||
| NamingStrategy namingStrategy() { |
There was a problem hiding this comment.
It would be nice to run this test with two different NamingStrategys. One should be one which significantly changes the names. I'm currently a little concerned the EntityRowMapper might only work because most databases ignore the case of names anyway by default.
There was a problem hiding this comment.
Changing case appeared like a cute way to at least visually verify it worked without breaking the query.
Unfortunately there appeared to be nothing to assert against. However, earlier unit tests may be adequate in that regard.
| * | ||
| * @author Greg Turnquist | ||
| */ | ||
| public interface Context { |
There was a problem hiding this comment.
I don't think this is used anywhere.
There was a problem hiding this comment.
Relic no longer needed.
| */ | ||
| public String getColumnName() { | ||
| return getName(); | ||
| return this.context.getNamingStrategy().getColumnName(this); |
There was a problem hiding this comment.
This is not the only place where we need to use the naming strategy. In SqlGenerator.cascadeConditions we use the table name of a referenced table and use it as a column name for a foreign key column. This should be controllable by the NamingStrategy as well.
Note that the FK reference goes in the opposite direction than the property (at least in the cases we have right now).
There was a problem hiding this comment.
That whole class appears to do entity.getTableName() and entity.getIdColumn which DOES use the naming strategy via BasicJdbcPersistentProperty.
There was a problem hiding this comment.
I was aiming at something else but didn't really explain it. The current version works fine for now. I'll create a separate ticket.
| private final NamingStrategy contextualNamingStrategy = new DefaultNamingStrategy() { | ||
| @Override | ||
| public String getSchema() { | ||
| return userHandler.get() + "."; |
There was a problem hiding this comment.
I think adding the dot after the real schema name should be handled by clients of the NamingStrategy
There was a problem hiding this comment.
If that complies with ANSI SQL definition for a Schema I agree. (I don't know. My experience is 99% oracle.)
There was a problem hiding this comment.
I'm almost as oracle sided, but what I have seen is: if there is a schema it is seperated with a dot from the table name. Some databases (DB2 I think) have a more complex construct, like schemas + databases. But still what ever stuff comes out of that becomes a prefix separated by a dot. So lets go with it at least until we learn more about the quirks of different databases.
| * | ||
| * @author Greg Turnquist | ||
| */ | ||
| public class FixedNamingStrategyUnitTests { |
There was a problem hiding this comment.
Here as well I think SqlGeneratorFixedNamingStrategyUnitTests is more appropriate.
gregturn force-pushed the issue/DATAJDBC-107 branch from 152bf52 to e681af5 Compare | @schauder I processed all your comments, then rebased against master, squashing the mods together. If you want to peek again, have at it. |
Created NamingStrategy and a default implementation that replicates the current solution. Several unit tests illustrates how to override the default and plugin a custom solution including a ThreadLocal, contextual one that could be user-based if, for example, Spring Security's SecurityContextHolder was used.
gregturn force-pushed the issue/DATAJDBC-107 branch from e681af5 to 469fb10 Compare 
| That's merged. |
We now use concatMap(…) instead of flatMap(…) when saving items to retain output item order.
No description provided.