Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement a NullEntityManager to prevent large NPE stacktraces when #3183

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@evanchooly
Copy link
Member

commented Jul 10, 2019

there are no classes marked up with @entity.

This is a case that bit me during a live demo. If there are no @entity classes, no EntityManager gets created/injected leading to unexpected NPEs trying to run queries. This patch introduces a dummy EM that essentially only returns empty Lists so that user code at least gets something usable.

I know there are probably plenty of conversations to have about both this idea and my particular implementation. This PR largely serves as a starting point for that conversation and, if I'm really lucky, its resolution as well. :)

Justin Lee
@gsmet

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

IMHO, this is going too far. I don't think generating mock objects in "production" code is a good idea.

Yeah, the first thing to do is to define your entities. But that sounds logical since you have to have their names, fields and so on to write your queries.

But I would have expected an injection error if don't have an EM, not a NPE. At least the error reporting should be improved.

@emmanuelbernard

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Right, at best it should only be activated in DEV mode but I wonder about your demo and steps you build stuff on.
Were you writing queries before even defining your entities? That feels a bit unnatural.

@emmanuelbernard

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Anyways I'm curious of the steps that led you there.

@stuartwdouglas

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

I don't understand how this really help's? NPE is not good, but IMHO Panache should throw an exception about the EM not being available.

If you actually tried to use a real entity manager before you have any entities you are always going to get an exception, we should make it meaningful, but IMHO we should definitely not just be faking it and returning null.

@evanchooly

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

In my case, I had the entity but forgot to put @Entity on it. My first thought was to blow up the build but didn't think anyone would go for that. After all there might be legitimate reasons for not having any entities defined (yet). And so I went with something like this. What if, instead of the Null* variants, if I throw an exception of some point when the EM comes back null. It could have a meaningful message that would be better than just a random NPE when trying to run a query.

@stuartwdouglas

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

If you have a Panache Repository then I think that should blow up the build if there is no EM (if it does not already). For the static method case I think it should fail with a meaningful error message. Returning null would be way more confusing IMHO. If you are calling a query method then you clearly expect there to be entities, we should make it clear that you have stuffed something up, not silently do something else.

@evanchooly

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

OK. I will update to throw an exception during the build phase about missing entities and repush.

@stuartwdouglas

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Only in the case of using the Repository. If there is just a class that extends PanacheEntity on the class path that is not annotated @entity then IMHO this should not be a build error, it should be a runtime error if you attempt to use it.

The difference is the repository is a CDI bean, while the (non)entity is not, and CDI beans are expected to have boot time validation applied to make sure they are valid.

@FroMage

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Only in the case of using the Repository. If there is just a class that extends PanacheEntity on the class path that is not annotated @entity then IMHO this should not be a build error, it should be a runtime error if you attempt to use it.

We could discuss this. IMO it has to have either @Entity or @MappedSuperclass on it. I don't think any other option is meaningful, is it @emmanuelbernard ?

If that's indeed the case, we could throw a meaningful error at build or runtime.

@emmanuelbernard

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

So yes we even considered adding @entity on a panache entity by default as it's "redundant". Not sure we agreed to or decided to not go that far.

@evanchooly

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Currently, the builds finds my PanacheEntity but no EntityManager gets injected because there is no @Entity on my class. So I'm tracking down where that decision to not register the EM for injection is made and will throw the exception there.

Justin Lee
@evanchooly

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

I'm not entirely sure of the placement or the exception type but this was the best place I could find to throw it. It's not in the panache bits but in the hibernate module which has me worrying it's not in the right place. AFAICT, the panache build steps didn't even run in the case of no entities so this felt like the best place to error out.

@emmanuelbernard

This comment has been minimized.

Hum, now I'm having second thoughts. @gsmet added this specifically to fix a problem were we started Hibernate and it was really bad for the demo. But that was pre ability to crash startup and still have quarkus:dev run and listen to requests.

So should we accept an EM with no entity, is that a thing?
The problem I had AFAIR is that I added hibernate-extensions and still wanted to work on the code with no exception. That we must allow.
Here you are actively using EM with no entity, that's where we want to throw an exception (or allow to run).

Thoughts @gsmet @Sanne

This comment has been minimized.

Copy link
Member

replied Jul 11, 2019

Having an EM which manages no entity types sounds reasonable to me. Code compiles, and you can inject such a thing, but you'll get the well known exception "not a managed entity" when trying to use it with a type it's not aware of.

Assuming we'll eventually have support for multiple Persistence Units, the error case should be similar to using the wrong PU for a specific type. Even better if someone can figure out an idea to make that type-safe but I doubt it :) Would be lovely to automatically bind the entity to a specific PU by flow analysis, but I'm dreaming.

@gsmet

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

It will really be a pain if, as soon as you add the extension, you can't start your app.

I agree we should improve the error message and not have NPE but I would keep the exception at runtime, not build.

@evanchooly

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

So what if we check for the null EM and rather using the NullX objects there we throw a PersistenceException ? We can log the missing @Entity bits at build time and then throw an exception at runtime.

Justin Lee
log the lack of @entity classes
move the exception to runtime
@evanchooly

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

I just pushed that change as mine was causing a test failure with the build time exception.

@gsmet gsmet self-assigned this Jul 17, 2019

@gsmet gsmet added this to the 0.20.0 milestone Jul 17, 2019

@gsmet
Copy link
Member

left a comment

Added some comments inline.

@@ -153,6 +155,7 @@ public void build(RecorderContext recorderContext, HibernateOrmRecorder recorder

if (!hasEntities(domainObjects, nonJpaModelBuildItems)) {
// we can bail out early
log.warn("No entities were found!");

This comment has been minimized.

Copy link
@gsmet

gsmet Jul 17, 2019

Member

I'm not a big fan of this warning as you will have it as soon as you add the ORM extension. I would remove it.

return Arc.container().instance(EntityManager.class).get();
EntityManager entityManager = Arc.container().instance(EntityManager.class).get();
if (entityManager == null) {
throw new PersistenceException("No EntityManager found. Do you have any Entities defined?");

This comment has been minimized.

Copy link
@gsmet

gsmet Jul 17, 2019

Member
Suggested change
throw new PersistenceException("No EntityManager found. Do you have any Entities defined?");
throw new PersistenceException("No EntityManager found. Do you have any JPA entities defined?");
@@ -68,7 +69,11 @@ public static void flush() {
// Private stuff

public static EntityManager getEntityManager() {
return Arc.container().instance(EntityManager.class).get();
EntityManager entityManager = Arc.container().instance(EntityManager.class).get();
if (entityManager == null) {

This comment has been minimized.

Copy link
@gsmet

gsmet Jul 17, 2019

Member

I would use isAvailable() here instead of doing the get() directly.

It makes things more readable IMHO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.