Skip to content

Conversation

@pavelda2
Copy link

Fix issue #10443

@pivotal-issuemaster
Copy link

@pavelda2 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 28, 2017
@pivotal-issuemaster
Copy link

@pavelda2 Thank you for signing the Contributor License Agreement!

@snicoll snicoll changed the title Ldap contextSource base property populating in EmbeddedLdapAutoConfig… Ldap contextSource base property populating in EmbeddedLdapAutoConfig Sep 28, 2017
@snicoll snicoll self-assigned this Sep 30, 2017
@snicoll snicoll added priority: normal type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 30, 2017
@snicoll snicoll added this to the 2.0.0.M5 milestone Sep 30, 2017
@snicoll snicoll closed this in 0fbc5de Sep 30, 2017
snicoll added a commit that referenced this pull request Sep 30, 2017
* pr/10444:
  Reuse spring.ldap.base in Embedded support
@snicoll
Copy link
Member

snicoll commented Sep 30, 2017

Merged, thank you!

@snicoll
Copy link
Member

snicoll commented Oct 2, 2017

This broke Spring Data's integration tests and, looking at the error, this change is way more invasive than I thought. Perhaps you could provide a solution that is opt-in instead?

@snicoll snicoll reopened this Oct 2, 2017
@snicoll snicoll closed this Oct 2, 2017
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed priority: normal type: enhancement A general enhancement labels Oct 2, 2017
@snicoll snicoll removed this from the 2.0.0.M5 milestone Oct 2, 2017
@snicoll snicoll removed their assignment Oct 2, 2017
@pavelda2
Copy link
Author

pavelda2 commented Oct 2, 2017

The problem is in integration test. Somebody set the spring.ldap.base=dc=springframework,dc=org property in test application.properties, but this property should have no effect before my improvement.

  1. It makes no sense to set whole ldap name uid=bob,ou=people,dc=springframework,dc=org in PersonRepositoryIntegrationTests.java when spring.ldap.base is set.
    (fix of findOneByName test)

  2. It makes no sense to set base in both - spring.ldap.base in application.properties and in the Person entity.
    (fix of the rest of tests)

From my point of view the test should be repaired.

Possible solutions:

  • Integration tests without base specified:
  • Integration tests with base specified:

@snicoll
Copy link
Member

snicoll commented Oct 2, 2017

@pavelda2 thanks for the feedback. Also paging @olivergierke that raised the regression in those tests.

It makes no sense to set whole ldap name uid=bob,ou=people,dc=springframework,dc=org in PersonRepositoryIntegrationTests.java when spring.ldap.base is set.

That's arguable I suppose, I wonder if there is a clear way to detect such use case. The error I saw was not very informative.

@vdubus
Copy link

vdubus commented Jan 19, 2018

I would suggest to use the value from spring.ldap.embedded.base-dn to set correctly the base information from LdapContextSource.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: declined A suggestion or change that we don't feel we should currently apply

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants