Skip to content

Conversation

@ttddyy
Copy link
Contributor

@ttddyy ttddyy commented Nov 18, 2018

In DataSourceJmxConfiguration with tomcat DataSource, it can only find DataSourceProxy if given DataSource is a direct child of it.

The check logic uses instanceof. When target DataSource is wrapped(delegated) or proxied, it cannot find DataSourceProxy.

Since DataSourceProxy#unwrap() always returns null, here cannot use this method to directly obtain DataSourceProxy.

In this PR, updated the check logic to perform the best effort to retrieve DataSourceProxy. If given DataSource is wrapped or proxied by spring, tries to unwrap or get target datasource recursively.

Prior to this commit, `DataSourceJmxConfiguration` with tomcat
`DataSource`, it can only find `DataSourceProxy` if the given
`DataSource` is a direct child of it.  Since it uses `instanceof`, it
could not find `DataSourceProxy` if the `DataSource` is
wrapped(delegated) or proxied.

This is because `DataSourceProxy#unwrap()` always returns null; thus
cannot use this method to directly obtain `DataSourceProxy`.

In this commit, updated the check logic to perform the best effort to
retrieve `DataSourceProxy`. If given `DataSource` is wrapped or proxied
by spring, tries to unwrap or get target datasource recursively to find
`DataSourceProxy`.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 18, 2018
@philwebb philwebb added type: bug A general bug for: team-attention An issue we'd like other members of the team to review and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 19, 2018
@philwebb philwebb added this to the 2.0.x milestone Nov 19, 2018
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Nov 21, 2018
@snicoll snicoll self-assigned this Nov 22, 2018
snicoll pushed a commit to snicoll/spring-boot that referenced this pull request Nov 23, 2018
Prior to this commit, `DataSourceJmxConfiguration` with tomcat
`DataSource`, it can only find `DataSourceProxy` if the given
`DataSource` is a direct child of it.  Since it uses `instanceof`, it
could not find `DataSourceProxy` if the `DataSource` is
wrapped(delegated) or proxied.

This is because `DataSourceProxy#unwrap()` always returns null; thus
cannot use this method to directly obtain `DataSourceProxy`.

In this commit, updated the check logic to perform the best effort to
retrieve `DataSourceProxy`. If given `DataSource` is wrapped or proxied
by spring, tries to unwrap or get target datasource recursively to find
`DataSourceProxy`.

See spring-projectsgh-15206
snicoll added a commit to snicoll/spring-boot that referenced this pull request Nov 23, 2018
snicoll pushed a commit that referenced this pull request Nov 26, 2018
Prior to this commit, `DataSourceJmxConfiguration` with tomcat
`DataSource`, it can only find `DataSourceProxy` if the given
`DataSource` is a direct child of it.  Since it uses `instanceof`, it
could not find `DataSourceProxy` if the `DataSource` is
wrapped(delegated) or proxied.

This is because `DataSourceProxy#unwrap()` always returns null; thus
cannot use this method to directly obtain `DataSourceProxy`.

In this commit, updated the check logic to perform the best effort to
retrieve `DataSourceProxy`. If given `DataSource` is wrapped or proxied
by spring, tries to unwrap or get target datasource recursively to find
`DataSourceProxy`.

See gh-15206
snicoll added a commit that referenced this pull request Nov 26, 2018
* pr/15206:
  Polish "Perform best effort to retrieve DataSourceProxy"
  Perform best effort to retrieve DataSourceProxy
@snicoll snicoll closed this in e424dfb Nov 26, 2018
@snicoll snicoll modified the milestones: 2.0.x, 2.0.7 Nov 26, 2018
@snicoll
Copy link
Member

snicoll commented Nov 26, 2018

Thanks for the PR @ttddyy. I've merged it with a polish commit.

@ttddyy ttddyy deleted the tomcat-datasource-jmx branch January 3, 2019 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: bug A general bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants