Skip to content

Conversation

ayudovin
Copy link
Contributor

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 25, 2018
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR but that's not really what I had in mind for this. Please see comments.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Dec 26, 2018
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

That's not what my first review was heading. Could you please reconsider the first review? I've also added some additional comments.

* property. When not specified will default to
* "org.hibernate.boot.archive.scan.internal.DisabledScanner".
*/
private String archiveScanner = "org.hibernate.boot.archive.scan.internal.DisabledScanner";
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the update but that's not what I meant. There shouldn't be any property at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

property was removed

@@ -121,6 +138,10 @@ else if (!result.containsKey(AvailableSettings.USE_NEW_ID_GENERATOR_MAPPINGS)) {
}
}

private void applyArchiveScanner(Map<String, Object> result) {
result.put(AvailableSettings.SCANNER, this.archiveScanner);
Copy link
Member

Choose a reason for hiding this comment

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

That's not checking if a user has set the property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added checking

@snicoll
Copy link
Member

snicoll commented Dec 28, 2018

@ayudovin that change broke Hibernate 5.2 support as indicated by the build failure. Have you noticed it?

@ayudovin
Copy link
Contributor Author

ayudovin commented Dec 28, 2018

@snicoll, yes, I try to find solution. Hibernate 2 doesn't have DisabledScanner and I haven't found a solution yet. As far as I understand we can't change version to 5.3, can we ?

@snicoll snicoll changed the title Consider disable Hibernate entity scanning for default JPA setup Disable Hibernate entity scanning for default JPA setup Dec 31, 2018
@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Dec 31, 2018
@snicoll snicoll self-assigned this Dec 31, 2018
@snicoll snicoll added this to the 2.2.0.M1 milestone Dec 31, 2018
@snicoll snicoll closed this in d0811b4 Dec 31, 2018
snicoll added a commit that referenced this pull request Dec 31, 2018
* pr/15565:
  Polish "Disable Hibernate entity scanning for default JPA setup"
  Disable Hibernate entity scanning for default JPA setup
@snicoll
Copy link
Member

snicoll commented Dec 31, 2018

Thanks @ayudovin. I've polished your contribution and fixed the Hibernate 5.2 problem as well.

@odrotbohm
Copy link
Member

I wonder if it makes sense to provide a simpler boolean flag as user facing property to avoid Hibernate internals leaking into user application configuration?

@snicoll
Copy link
Member

snicoll commented Dec 31, 2018

@odrotbohm there is not configuration exposed at all so I am not sure what you mean there. If you don't specify anything, we use the disabled scaner impl (which, I believe, matches your original report).

@odrotbohm
Copy link
Member

Oh, and does it make sense to use the disabled scanner by default. I guess an average Boot user doesn’t expect any Hibernate-driven scanning to run in the first place anyway?

@snicoll
Copy link
Member

snicoll commented Dec 31, 2018

Oh, and does it make sense to use the disabled scanner by default.

Is that a question? If that is, you lost me as you created an issue to request that scanning to be disabled by default (unless I misunderstood it). If disabling the scanner is not 100% safe, we should revert this and clarify the scope of the issue you've created.

@odrotbohm
Copy link
Member

Okay, maybe the changes view is messed up then. I see tests that set a pretty complicated property that directly configured the scanner. Given our experience with Hibernate those things change quite frequently so that I wondered whether something like .…disable-hibernate-scanning to either be true or false allow us to mitigate potential changes.

@odrotbohm
Copy link
Member

I was just looking at what the changes tab provided on mobile view and that doesn’t seem to contain anything hinting at what default you chose.

@snicoll
Copy link
Member

snicoll commented Dec 31, 2018

There is nothing in your request that indicated this should be configurable. We can introduce a property but that doesn't match the description of #15321. I've already indicated that if disabling the scanner in the context of a Spring Boot auto-configured app wasn't safe, I think the scope of the issue should be clarified (with a potential reopen).

As for the "pretty complicated" property, it's just the standard Hibernate property with a prefix to help us grab it.

@odrotbohm
Copy link
Member

Fine, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants