Improve FailureAnalyzer for embedded datasource #11953

pkostrzewa · 2018-02-07T16:50:46Z

Moved and refactored logic related to failure analysis from exception to DataSourceBeanCreationFailureAnalyzer.

pkostrzewa · 2018-02-07T18:02:29Z

...onfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceProperties.java

@@ -520,39 +520,39 @@ public void setProperties(Map<String, String> properties) {

 	}

-	static class DataSourceBeanCreationException extends BeanCreationException {
+	class DataSourceBeanCreationException extends BeanCreationException {


I'm not sure whether I should remove static modifier. Maybe it would be even better to move this class outside of DataSourceProperties?

pkostrzewa · 2018-02-08T08:52:04Z

@snicoll any feedback would be appreciated :)

snicoll

Thanks for the PR but that change doesn't need to be that involved. The exception doesn't need to change but perhaps it can expose extra information.

The real deal is the failure analyzer. We need to reformat the description as described in the related issue.

snicoll · 2018-02-08T11:42:45Z

.../java/org/springframework/boot/autoconfigure/jdbc/DataSourceBeanCreationFailureAnalyzer.java

 	}

+	private String createDescription(DataSourceBeanCreationException cause) {


I don't think that's what I had in mind. What we need to do here is check if "NONE" is active and change the description accordingly.

You mean that such detailed description should be provided only in case when connection stands for "NONE"?

No that's not what I meant. I think we need to take a step back. Can you rework your PR to change the exception to expose the EmbeddedDatabaseConnection? Then you can adapt the message when it is NONE. The change to fix that issue must not be that large. Let me know if that works for you.

It's ok for me. Actually I've reworked it similarly .

snicoll · 2018-02-08T11:43:06Z

...onfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceProperties.java

 		}

-	}
+		public Environment getEnvironment() {


You can't expose such thing in an exception.

snicoll · 2018-02-08T11:43:14Z

...onfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceProperties.java

-			}
-			message.append(".");
-			return message.toString();
+		public EmbeddedDatabaseConnection getConnection() {


snicoll · 2018-02-08T11:43:50Z

...onfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceProperties.java

 		DataSourceBeanCreationException(EmbeddedDatabaseConnection connection,
 				Environment environment, String property) {
-			super(getMessage(connection, environment, property));
+
+			super("Cannot auto-configure DataSource. ");


If you want to pursue this, the message should be provided by the caller, you can't hard-code it like that.

pkostrzewa · 2018-02-09T12:46:24Z

There are two options:

expose connection from DataSourceBeanCreationException (https://github.com/spring-projects/spring-boot/pull/11953/files/ec9cfa405dd716e97e5c54d0adb23e7b78f7c72c#r166909465 - you pointed out that might not be good idea to expose such information) and then build proper messages in failure analyzer depending on whether connection is NONE
expose only description and action already created inside of exception and don't change them in failure analyzer (hides details about exception)

Which one do you prefer @snicoll ?

snicoll · 2018-02-09T13:35:58Z

@pkostrzewa Thanks. I think that we need to revert the logic and build the message in the failure analyzer as you've done initially. But I'd like the changes to be a bit more focused. Here is what you can do:

You can inject the Environment in a FailureAnalyzer if you implement EnvironmentAware (FailureAnalyzer should be able to inject the environment #11569)
We can expose the EmbeddedDatabaseConnection in the exception as a getter
We can simplify the message to use the "property" part. We can use that then in the failure analyzer as a support to build the description message, the two message can be "Failed to determine a suitable driver class" and "Failed to determine a suitable jdbc url". We can build those message as is in the caller rather than computing them in the exception

So our exception takes now a message (see above) and an EmbeddedDatabaseConnection. Once you have that, we can craft a better error message. And we can have a special case for NONE to indicate that no embedded database was found on the classpath.

pkostrzewa · 2018-02-09T19:17:16Z

EmbeddedDatabaseConnection exposed as a getter in exception
DataSourceBeanCreationFailureAnalyzer implements EnvironmentAware so that current environment can be accessed
DataSourceBeanCreationException takes an connection and a message
Available actions are computed based on whether the connection type is EmbeddedDatabaseConnection.NONE
DataSourceProperties class in no longer aware of Environment as it was only passed as an exception argument

Currently there are no tests that covers all edge cases - maybe we should take them into consideration?

pkostrzewa · 2018-02-10T15:59:11Z

@snicoll any feedback?

See gh-11953

* pr/11953: Polish "Improve FailureAnalyzer for embedded datasource" Improve FailureAnalyzer for embedded datasource

snicoll · 2018-02-12T10:13:40Z

@pkostrzewa this is better but not quite there yet. Given the back and forth, I've polished your last proposal, see 2f13449

Improve FailureAnalyzer for embedded datasource Fixes gh-8029

aef7279

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 7, 2018

snicoll self-requested a review February 7, 2018 17:03

Fix checkstyle errors

d25dd0f

pkostrzewa commented Feb 7, 2018

View reviewed changes

pkostrzewa added 2 commits February 7, 2018 19:06

Restored accidentaly removed message to description

a8e0731

Fixed failing test

ec9cfa4

snicoll requested changes Feb 8, 2018

View reviewed changes

Fixes

211c4d0

snicoll self-requested a review February 10, 2018 14:08

snicoll self-assigned this Feb 10, 2018

snicoll mentioned this pull request Feb 12, 2018

Improve FailureAnalyzer for embedded datasource #8029

Closed

snicoll added type: enhancement A general enhancement priority: normal and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 12, 2018

snicoll added this to the 2.0.0.RC2 milestone Feb 12, 2018

snicoll changed the title ~~Improve FailureAnalyzer for embedded datasource Fixes gh-8029~~ Improve FailureAnalyzer for embedded datasource Feb 12, 2018

snicoll pushed a commit that referenced this pull request Feb 12, 2018

Improve FailureAnalyzer for embedded datasource

e66745a

See gh-11953

snicoll closed this in 2f13449 Feb 12, 2018

snicoll added a commit that referenced this pull request Feb 12, 2018

Merge pull request #11953 from pkostrzewa:gh-8029

fba2615

* pr/11953: Polish "Improve FailureAnalyzer for embedded datasource" Improve FailureAnalyzer for embedded datasource

snicoll removed their request for review February 12, 2018 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve FailureAnalyzer for embedded datasource #11953

Improve FailureAnalyzer for embedded datasource #11953

Uh oh!

pkostrzewa commented Feb 7, 2018 •

edited

Loading

Uh oh!

pkostrzewa Feb 7, 2018

Uh oh!

pkostrzewa commented Feb 8, 2018

Uh oh!

snicoll left a comment

Uh oh!

snicoll Feb 8, 2018

Uh oh!

pkostrzewa Feb 8, 2018

Uh oh!

snicoll Feb 8, 2018

Uh oh!

pkostrzewa Feb 8, 2018

Uh oh!

snicoll Feb 8, 2018

Uh oh!

snicoll Feb 8, 2018

Uh oh!

snicoll Feb 8, 2018

Uh oh!

pkostrzewa commented Feb 9, 2018 •

edited

Loading

Uh oh!

snicoll commented Feb 9, 2018 •

edited by philwebb

Loading

Uh oh!

pkostrzewa commented Feb 9, 2018 •

edited

Loading

Uh oh!

pkostrzewa commented Feb 10, 2018

Uh oh!

snicoll commented Feb 12, 2018

Uh oh!

Uh oh!

		}

		private String createDescription(DataSourceBeanCreationException cause) {

Improve FailureAnalyzer for embedded datasource #11953

Improve FailureAnalyzer for embedded datasource #11953

Uh oh!

Conversation

pkostrzewa commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkostrzewa commented Feb 8, 2018

Uh oh!

snicoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkostrzewa commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snicoll commented Feb 9, 2018 • edited by philwebb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkostrzewa commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkostrzewa commented Feb 10, 2018

Uh oh!

snicoll commented Feb 12, 2018

Uh oh!

Uh oh!

pkostrzewa commented Feb 7, 2018 •

edited

Loading

pkostrzewa commented Feb 9, 2018 •

edited

Loading

snicoll commented Feb 9, 2018 •

edited by philwebb

Loading

pkostrzewa commented Feb 9, 2018 •

edited

Loading