Log a warning on startup when spring.jpa.open-in-view is enabled but user has not explicitly opted in

[vpavic]

Considering OSIV/OEMIV is widely considered an anti-pattern, OpenEntityManagerInViewInterceptor should IMO not be enabled by default. Rather than that it should be opt-in.

If this proposal isn't accepted at the very least the default behavior should be stressed properly in the documentation. Currently the only reference is in configuration properties appendix.

[wilkinsona]

I agree. I think we should change the default in 2.0.

[wilkinsona]

Closing in favour of the PR (#7125)

[odrotbohm]

Considering OSIV/OEMIV is widely considered an anti-pattern, …

Would anyone care to elaborate? I definitely see it's a controversial topic, but I think that "widely considered anti-pattern" is quite a bit of a stretch. It had been an anti-pattern in the days (< Spring 2.0) when O(S|E)IV automatically meant opening a transaction, too, but whenever I see someone (esp. from the CDI camp) arguing O(S|E)IV is a bad idea, the next step that usually follows is exposing a request scoped EntityManager which is exactly the same thing with the same advantages and problems.

So I'd love to learn what's wrong with the default setting? Judging from the projects I see, I guess everyone would then just turn it on manually.

[vpavic]

IMO if you have a transactional unit-of-work whose result of execution requires further fetching from database that occurs after the aforementioned unit-of-work has ended I don't know how adding behavior that supports such data access pattern can be described not being an anti-pattern. Same for not having well defined transactional boundaries. Anyway not to go any further here my thoughts on this topic have been covered in @vladmihalcea's blog post better than I could express them myself. I respect that everyone has their preferences when it comes to data access but I cannot agree this is not an anti-pattern.

So I'd love to learn what's wrong with the default setting? Judging from the projects I see, I guess everyone would then just turn it on manually.

Well, my experience is just the opposite. 😄

What's wrong with the current default settings is that adds behavior that's too opinionated to address the complex subject of lazy loading of associations. Even worse, it does so completely silently, because as described in my original comment, its only mention is in the configuration properties appendix. I'd also argue this is bad for novice developers as it completely hides lazy loading concerns from them, rather then have them face such concerns and decide how to address them after educating on the topic.

If someone wants OSIV, they'll enable it with a single configuration property first time they face the LazyInitializationException.

[odrotbohm]

IMO if you have a transactional unit-of-work whose result of execution requires further fetching from database that occurs after the aforementioned unit-of-work has ended I don't know how adding behavior that supports such data access pattern can be described not being an anti-pattern.

I can't really follow the sentence here. That's circular reasoning, isn't it? Because you describe it as anti-pattern you can't see how it cannot be described as such. Hm.

I just re-read Vlad's blog and still remain unconvinced. Actually, I agree with everything he writes there. Still, there's nothing that inherently breaks code when an O(S|E)IVF is in place. Yes, its usage can cause suboptimal query performance and all other kinds of suboptimal effects if you don't know what you're doing. The thing is: if you start to argue that way, you basically have to shun JPA entirely. Yes, it's quite easy to suboptimally configure mappings, query executions etc. Still, I'd fix those problems when they actually occur to become some.

Quite the opposite picture without an O(S|E)IVF: fundamental things you naively (read: not being a JPA/Hibernate expert like Vlad, you, me (?)) expect to work — like traversing a property (e.g. while rendering a view) — will just stop working. Now you can of course go ahead and raise the experts finger, tell people to read Vlad's blog and then do the "right thing"™. I'd argue most people will rather do one of two things: activate O(S|E)IVF again and just live with the slight decrease in performance, or — even worse — rather extend their transaction boundaries as this might seem as the even more easy thing to do. I mean, why would you use an OR mapper if not for convenience. If you want to optimize the heck out of your relational data access, you're probably down to SQL anyway.

I personally have never seen any real problems being caused by the EntityManager (and thus the connection) being held open during view rendering. If that really becomes a performance problem: there's an easy way to turn that off. Not being able to traverse a model in the view and being forced into DTOing every entity is a much more cumbersome default. Note that one of Boot's core goals is to provide a great developer experience. Following the "but it might not be the most optimal way of working", we would never have started with things like Spring Data (JPA), as you can clearly create non-optimal queries OOTB if you're not deeply into the store characteristics.

I don't think O(S|E)IV is too opinionated, as it's not the thing that imposes the opinion. JPA does in the first place. O(S|E)IV just causes fundamental things to continue to work as non-JPA experts expect them to work. I am not even saying O(S|E)IV is an optimal solution but the current setup is trading a working and potentially suboptimal OOTB experience over an experience that requires very deep understanding and looks like it's not working OOTB.

So I end up wondering what real benefit hanging the default has I am not aware of any real problems stemming from the current default that have been brought up here and we'd basically just cater a different opinion, with the very likely chance that if the default was changed, the other camp would just again ask for it to be reverted again.

[vladmihalcea]

To use or not to use OSIV is an architectural decision, and, in the good spirit of DevOps, the DBA must be involved when making all these decisions because OSIV can become a performance issue only in a production environment, not on a developer machine.

From a developer perspective, OSIV is very attractive since it can boost productivity. However, if you're not involved in tuning the enterprise system and scaling it with the ever increasing incoming throughput, you'll have a very optimistic view on OSIV or temporary Session anti-pattern.

Both Spring and Hibernate offer some opinionated options, but, in the end, it is the responsibility of the DevOps team to decide whether a given feature makes sense for their particular system.

Therefore, this option should be explicitly activated.

[vpavic]

@olivergierke I'm sorry but your take on this with statements like if you start to argue that way, you basically have to shun JPA entirely sounds like it's all or nothing with JPA. IMO where it (JPA/Hibernate) excels the most are the write scenarios, persisting complex graphs with added value in features like locking mechanisms. OTOH with reads you have so many options to choose from, and I (as well as many others) prefer to be explicit with what I fetch and how exactly I fetch it from the database. Such things should IMO not be dictated by your mappings.

Also I have a problem with statement that If that really becomes a performance problem: there's an easy way to turn that off - you won't get yourself out of that kind of a problem by simply turning off OSIV. If you had relied on OSIV while developing your app and you have to turn it off at some point down the road, that will almost certainly require a fair share of refactoring of your data access related code.

[odrotbohm]

What I was trying to get across that JPA is already a trade-off. A quite huge one, especially favoring developer convenience over performance / efficiency (as otherwise you'd use more low-level technology like SQL in the first place). So blaming O(S|E)IV for producing suboptimal results in some cases is quite a stretch.

In case you find any of the effects O(S|E)IV are really becoming a problem in your application, you can switch of the defaults, still use a manually configured O(S|E)IV bound to the paths that just work fine with it and tweak the problematic scenarios. Heck, depending on what problem you really run into, you can still use Vlad's great advice to optimize mappings and queries even with the O(S|E)IV present.

I'd always rather act on problems when they appear rather than hastily DTOing my entire codebase in anticipatory obedience. Isn't the latter a great example of premature optimization? Also, always remember that with the current setup there's nothing preventing you from doing all the things you'd like. We solely argue OOTB developer experience, a default, and I'd argue that this should be less invasive to the overall code design than what it'd be if what's suggested is applied.

[nfrankel]

I read the whole thread and I can only agree that OSIV is an anti-pattern.

My point here is not about performance but about architectural design. I mean it's called Open Session in View for a reason, right? Not only do we handle database-related stuff in the view layer - which is wrong from a conceptual point of view, what happens if an exception happens while the response stream is being written? The page has only partially being served to the client so the stack trace gets written here as well... There's no way to handle nicely it.

That's the original sin of OSIV: it breaks in a very bad way one of the basics of software development - separation of concerns.

(Bragging rights: I might even have been one of the first to describe OSIV as an anti-pattern as such back in 2010 when it was all "it's described in the Hibernate book so you're plain wrong").

My 2 cents.

[odrotbohm]

…what happens if an exception happens while the response stream is being written?

How is that an O(S|E)IV specific problem? Exceptions can be caused by anything.

That's the original sin of OSIV: it breaks in a very bad way one of the basics of software development - separation of concerns.

That feels quite constructed to me. Following that train of thought, you can argue that an @Transactional is violating this concern as well as you mix a technical thing with your business logic. It feels like you're trying to decouple for the reason of decoupling, and by that actually create incentives for users to create much more technical code than probably necessary. At some point you have to connect things to things, don't you?

So again, what hazardous problems are caused by O(S|E)IV being the default, that are introduced by O(S|E)IV actually, and not some upstream technology decision? I can probably stop repeating that I totally see the potential downsides of using it 🙃. I just think the benefits of letting it be the default — again, nobody's forcing anyone to actually use it — introduces totally outweigh the drawbacks that could occur under certain conditions.

[vpavic]

@olivergierke I don't think that hastily DTOing my entire codebase in anticipatory obedience was anywhere suggested as preferred approach so I don't see why it's necessary to go to extremes like that, similarly as with you basically have to shun JPA entirely.

Speaking of invasive, IMO if anything's invasive it's enabling OSIV by default. With off by default at first sight of LazyInitializationException, and that will happen early in your development lifecycle, you'll be faced with a decision whether to add a single config property to enable OSIV or not. Otherwise you're hiding away an important aspect of underlying peristence technology and architectural decision, and by doing so increase the chances it will take some time for someone will realize things don't work exactly as they thought they do.

[odrotbohm]

Following that train of thought, there would never be an argument for activating things by default. You're basically arguing that we should enable JPA and effectively leave the user with property traversal on domain objects being broken.

[nfrankel]

…what happens if an exception happens while the response stream is being written?
How is that an O(S|E)IV specific problem? Exceptions can be caused by anything.

The exception is not the point, the fact that it cannot be handled cleanly is. If an exception occurs in one of the "deep" layers, it can bubble up to an exception handler. If the same occurs while writing the response stream, it cannot as part of the page as already been committed.

[odrotbohm]

And that problem can only appear when using an O(S|E)IV? 😳

[nfrankel]

And that problem can only appear when using an O(S|E)IV? 😳

I let you check the logical fallacies list.

I don't understand why this issue is so important that different valid arguments from different people get just discarded so easily. I've said my piece, others as well. As far as I understand, the decision has been made already?

[odrotbohm]

I don't understand why this issue is so important…

Context is key: this ticket asks for a different default in O(S|E)IV filter, hence we collect arguments for that. That an exception during the rendering phase of a view is a general problem, totally unrelated to whether O(S|E)IV is enabled by default or not, right?

We're getting drawn into arguments of the consequences of using or not using O(S|E)IV. But that's not really the context of this ticket, is it? The default will not decide whether people ultimately end up using a certain approach or not. It's about whether the current default is in line with Boot's goals.

[vladmihalcea]

OSIV is the "blue pill". You give it to Spring Boot users, and, by the time they realize that hiding the LazyInitializationException is causing performance issues, it's going to take a lot of effort to switch from OSIV to fetch data on a per-business use case basis.

However, I understand why you'd enable OSIV by default. The easier to use, the more adoption you'll get for Spring Boot.

We could have done the same with the hibernate.enable_lazy_load_no_trans configuration property. We could just enable it by default and make Hibernate behave like EclipseLink - hiding the LazyInitializationException under the carpet.

But we didn't, and that hasn't affected Hibernate popularity.

For JPA and Hibernate consultants, enabling OSIV by default in Spring Boot is actually a gift because they will have more opportunities for performance tuning Spring/Hibernate applications.

So, it's indeed a tough decision.

[philwebb]

After a lot (and I do mean a lot) of discussion we've decided to leave things as they are. The primary reason is that people upgrading are likely to face very subtle issues that only manifest themselves in certain circumstances.

If we were starting from a clean slate we may well have picked a different default, but we think the pain of changing the default (even at a major release) is going to cause more bugs than leaving things as they are.

[martin-g]

Spring Boot is just Srping after all

Yes, in plain Spring you have add the additional Servlet Filter yourself. Not as simple as in Boot but again very easy!

My experience is exactly the opposite - I haven't seen a Spring application with Hibernate that doesn't use OSIV :-/
I don't say that using OSIV is better! I just say that it is widely spread (according to my experience!).
In any application that strives for performance I'd just use JdbcTemplate.
Maybe org.springframework.orm.hibernate5.HibernateTemplate should be promoted ?! (I haven't used it so far).

[jkubrynski]

@wilkinsona Is Spring Boot targeted for beginners or to create production-ready applications? If it's a project for sandboxing and 5-minute live-coding on presentations then leaving OSIV enabled is a great idea. But deploying OSIV to production is a terrible idea.

[beikov]

For the read concerns you can use a projection tool like e.g. Blaze-Persistence Entity Views or if you only need simple projections Spring Data Projections should be enough too.
I am currently working on also handling the write concern with Entity Views, but this is still in development. If you provide me a sample app using OSIV I can adapt it and show you how to implement a DTO approach.

[wilkinsona]

Is Spring Boot targeted for beginners or to create production-ready applications?
@jkubrynski I'd guess that's a rhetorical question, but I think it's worth addressing anyway.

Spring Boot is obviously targeted at both, which is why this isn't an easy decision to make. There's no point in making something great for production if people give up and choose a different stack because the getting started experience is poor.

The ideal here would be for the getting started experience to have a gentle learning curve and for the curve to continue to be gentle as you move towards production. Right now, the curve either has to be really steep at the beginning (OSIV disabled by default), or potentially really steep later on (OSIV enabled by default and it causes performance problems). Potentially really steep later on is the lesser of those two evils, IMO.

Perhaps we can strike more of a balance while still leaving OSIV enabled by default? I wonder if it would be possible to detect when it's prevented a LazyInitializationException and log a warning. That way people would get an easy getting started experience, but would also be warned about potential problems.

[vpavic]

The ideal here would be for the getting started experience to have a gentle learning curve and for the curve to continue to be gentle as you move towards production. Right now, the curve either has to be really steep at the beginning (OSIV disabled by default), or potentially really steep later on (OSIV enabled by default and it causes performance problems). Potentially really steep later on is the lesser of those two evils, IMO.

@wilkinsona I understand and agree that Boot should target both groups, and that striking a balance is often times difficult, however I strongly disagree with the part in bold.

If you consider the topic of project's life cycle I think it should be obvious that the damage is much bigger if anyone runs into problems due to use of OSIV at later stages of the project. Especially considering the projects of enterprise-ish size, the effort to move off OSIV can be quite costly and potentially catastrophic to the project (delay of delivery dates, production issues, etc.). I don't know about others who participated in this discussion, but on every project I've worked on in my career the maintenance phase remained among my responsibilities as well so I care a lot about these things.

Regarding the concise and beginner-friendly example of OSIV-less approach, I think the 2 or 3 @vladmihalcea's blog posts that have been linked here do explain things in concise and beginner-friendly way. Do you perhaps insist on code based example that would be a part of Boot's codebase as a sample project?

Just to make it clear, I'm not trying to force anyone not to use OSIV or promote the data access approach that I use. It's just that I believe that having it enabled is a really bad default that has severe architectural implications, and as such it should be left for users to be explicit about whether they want it or not. I cannot recall any other of Boot's opinions that has such architectural impact. In fact, there are a number of other places where Boot has moved or is moving from having something on by default to requiring users to enable it using configuration property (e.g. spring.session.store-type, database initializers) and I don't believe that made Boot any less beginner friendly.

[aahlenst]

In my opinion having OSIV enabled without being aware of it is a problem. Why not leaving it on by default in 2.0, printing a big warning to the console that prompts the developer to explicitly opt-in? Everything accompanied by some concise docs that explain the implications of OSIV=true/false. The resulting behavior would be:

JPA is not enabled: No warning
JPA is enabled, no OSIV setting declared (OSIV is on): Warning is emitted
JPA is enabled, OSIV=true: No warning
JPA is enabled, OSIV=false: No warning

[wilkinsona]

@aahlenst Thanks for the suggestion. Let's see if we can implement what you've proposed in M6.

[beegor]

IMHO, enabling OSIV by default does disservice to beginners. It hides the way Hibernate/JPA really works with collection mappings. one-to-many and many-to-many relations which are lazy by default will behave like eagerly fetched. Beginners who are familiar with fetching modes would be confused by absence of LazyInitializationException and maybe dig more , but many of them will be happy that things just work magically and likely pretty confused on some future project that does't have OSIV configured.
I always felt about OSIV as of violation of "separation of concerns" principle, but it's not even the point here.
The point is OSIV should be explicitly enabled only by someone who knows what it is and what it does. Let the beginners learn Hibernate/JPA pitfalls early, don't hide it.

[vladmihalcea]

It's worth adding this StackOverflow question where someone asks for explicitly disabling OSIV because the magically handling the LazyInitializationException can hide bugs.

[odrotbohm]

Let me use this opportunity to point out that we've now reached a point where a user has declared a class with a field and an accessor method returning that field. That user is now actually surprised that this code does what it's declared to do and does not throw an arbitrary technical exception.

And this is considered the "wrong" behavior and we use that as an argument to actually change that.

[vladmihalcea]

a user has declared a class with a field and an accessor method returning that field

@olivergierke Are you talking about the same StackOverflow question?

There is no mapping being shown there.

[odrotbohm]

There is no mapping being shown there.

Let's see if we find a Hibernate / JPA expert that can educate us on what code looks like that usually throws LIEs (funny abbreviation, eh?). Anyone?

[vladmihalcea]

I'm not sure if I qualify as a Hibernate / JPA expert, but I wrote my 2 cents on this article on my blog, the best way to handle the LazyInitializationException LazyInitializationException.

Let me use this opportunity to point out that we've now reached a point where a user has declared a class with a field and an accessor method returning that field. That user is now actually surprised that this code does what it's declared to do and does not throw an arbitrary technical exception.

I have a way better analogy:

1. The user has a banking account with a balance of 50$.
2. The user wants to withdraw 60$, and instead of getting an InsufficientFundsException, they will get the 60$ without knowing that there's an overdraft fee for the extra 10$.
3. The longer it takes for the user to realize they have a debt to pay, the costlier it will become.

The whole point of this thread was to let the user know about this issue, as long as OSIV is still enabled by default.

Hiding a LIE (funny abbreviation, eh?) is not the same as telling the truth,

Anyway, I don't think this the discussion is going anywhere, so there is no point in continuing it.
I'll focus on making people aware of this issue so that they can take the best decision whether they want to have OSIV enabled or not.

[odrotbohm]

That’s neither a better analogy nor an analogy at all to what the original reporter in that StackOverflow post asked. But it’s in line with a lot of the “making up things from thin air” presented in this thread. An LIE is not a business exception, it’s not triggered by any implementation of a business rule. It’s a technical distraction from your actual business problem.

My point was not about how to handle LIEs, but that code you can perfectly reason about is considered “broken” if it works the way it’d work in plain Java, without JPA in place. OEMIV is one means to solve that with well-known pros and cons. Heck, OEMIV is not even about handling exceptions, it’s about preventing them. It’s not about correctness, it’s about efficiency, convenience and optimizations. That’s a world of a difference. The thread here wants to imply something is horribly broke, where it’s actually about a trade-off between convenience and efficiency. We need proper data points to evaluate that balance but more on that below. The issue that triggered my comment was, that – I guess without a lot of people not realizing - the SO post was more proof of the underlying problem than that it was how to apply one of the solution options.

Anyway, I don't think this the discussion is going anywhere, so there is no point in continuing it.

We’ve reached said point a while ago when the proponents of the suggested change boasted that an alternative implementation to what the current default allows would be oh so very simple, but then constantly fail to provide any meaningful example of what this would look like, even after repeated requests to provide some code. However, that chance is still given. Unfortunately, so far, we’ve seen nothing but blowing smoke instead. I can point to a gazillion of SO threads in which users were caught by surprise about LIEs and where OEMIV was a decent solution for. But yeah, that’s not moving this discussion forward a bit. I guess my ironic remark was just my way of pointing out the pointlessness of that.

Shortcutting this: I guess, we should move forward by finally seeing examples what an alternative user application implementation would look like, or resolve the discussion.

[vladmihalcea]

finally seeing examples what an alternative user application implementation would look like.

It's simple.

Instead of:

@Transactional
public List<PostComment> getPostComments(String review) {
	return entityManager.createQuery(
		"select pc " +
		"from PostComment pc " +
		"where pc.review = :review", PostComment.class)
	.setParameter("review", review)
	.getResultList();
}

They would write:

@Transactional
public List<PostComment> getPostComments(String review) {
	return entityManager.createQuery(
		"select pc " +
		"from PostComment pc " +
		"join fetch pc.post " +
		"where pc.review = :review", PostComment.class)
	.setParameter("review", review)
	.getResultList();
}

It's just one extra JOIN FETCH for each @ManyToOne or @OneToOne associations being needed further up the call stack.

Or they can use Entity Graph, if don't write the JPQL or Criteria API query themselves and rely on Spring Data for that.

For collections, they can JOIN FETCH up to one collection, otherwise a Cartesian Product is generated.

So, any extra @OneToMany or @ManyToMany would require to be navigated before returning from the @Transactional block.

However, initializing more than one collection can easily lead to an N+1 query issue, and that's the case even if OSIV is disabled or not.

[odrotbohm]

And what in the world is preventing anyone from writing the query the way you suggested with the current default? I think it’s a decent way of selectively optimizing queries while still retaining the convenience of everything not explicitly optimized still working as the written code suggests (getPost() returning the Post no matter whether it’s currently loaded or not).

[vladmihalcea]

And what in the world is preventing anyone from writing the query the way you suggested with the current default?

Why would they if it works magically like this:

List<PostComment> findByReview(String review);

With a single Spring Data method I can:

• fetch the List<PostComment> by review when I only need data from PostComment
• fetch the List<PostComment> by review when I need data from PostComment and from the Post as well

Why would a developer want to write separate methods and think about fetching data efficiently when their priority is addressing issues from the current Sprint log?

[wilkinsona]

I have locked this issue, for now at least, as the discussion no longer seems to be productive. We’ll add the logging suggested above by @aahlenst.

[wilkinsona]

Closed by 5aa6630.

[xak2000]

Why would a developer want to write separate methods and think about fetching data efficiently when their priority is addressing issues from the current Sprint log?

Yes, and they shouldn't!
There is profiling, logging and monitoring for this.

If it is a not a high load service, then N+1 problem can never be noticed. Yes, it can work suboptimal, but who cares if it runs 10 queries/hour (1 main query and another 9 because of N+1 "problem" 😄)?

If it is a high load service, than it should have a profiling, monitoring, logging etc, so this performance degradation will be noticed, then SQL log will be enabled and N+1 problem will be noticed. And only then some decision will be made:

• Always use JOIN FETCH (for example, if the performance will be acceptable even in case when we don't need related data).
• Write another method with JOIN FETCH and use one or another according to which fields of model will be needed (it is preferable from performance point of view, but is bad from convinience, clean-code and "service layer should have no knowledge about view" point of view. All these getUser, getUserWithPorts, getUserWithPortsAndLines etc :( ).
• Leave it alone because this N+1 execution path appears only in very specific (rare) cases (users rarely click "show more" button).
• Refactor to use specialized DTOs (can be part of another approach).
• Another approach.

And all of them are possible without disabling OSIV.
I prefer optimizing the bottlenecks, not all possible paths.
Or else why should I use ORM in first place if I would always manually craft explicit queries for each and every use-case? Even these cases, when optimization is not required at all and lazy fetching is acceptable.

Maybe it will be more convinient to use something like JOOQ than JPA then?

---

Another concern is what to return from these "specialized" methods?
Like in getPostComments example above. If this is the only method returning List<PostComment> then we can just make post field EAGER. If not, then there is another method without join fetch pc.post, like getPostCommentsWithoutPost, right?

Should we really return List<PostComment> then? If we get the list using getPostComments, then we can call list.get(0).getPost() method. If we get the list using getPostCommentsWithoutPost, then (without OSIV) we can't do this (LIE) despite the fact that both methods return the same models. So as a developer I should always be aware that PostComment can containt the post or not contain the post. 🤦‍♂️ And I can never be sure if I can call getPost() method on it. It could be not clear where this list comes from in the code, so sometimes it could be very hard to understand what kind of PostComments (with or without post) in the concrete list. Partially filled models are more evil then OSIV IMHO.

So to prevent this we forced to always use DTOs then. Different DTOs of same model for different use-cases (feel the pain 👿 ).

And if we decide to do this, then OSIV just does nothing. Nothing good, but also nothing bad. Lazy loading will not happen anyway. Open session will still be open, but DTOs doing nothing with it.

So, my opinion is that, there is no one and only one "right" way to do the things. All have its drawbacks.

And if we want totally ignore the ORM's ability to lazily load relations, then maybe we should introduce a hibernate.use-the-joins-luke flag. :) and throw LIE on any not loaded field access attempt even when em is still open. :)