Calling load seems unconventional, I believe it should be get()
I fully agree!
Getting a persistent by PK is not a finder, it makes sense to throw an ObjectNotFoundException. Is there a use case where the provider id is known but does not exist? Maybe it’s a sign of something not right (an FK missing perhaps).
getDomainObject() by ID or UUID use hibernates
get() and dont throw exceptions in case of non existence.
@darius do you maybe know why this is the case for provider?
related ticket https://issues.openmrs.org/browse/TRUNK-5153
I can’t think of any reason why I would have intentionally done this 6
years ago. I assume we can switch to
Do you prefer a NullPointerException instead of an ObjectNotFoundException? I would say that this case almost never happens (an entity is deleted when it’s open in another user session).
We have ~18 HibernateXXXDAO, of which only one uses hibernates
load() method. I would care more about consistency.
You could change all the 18 and be consistent.
I’d think we shouldn’t be throwing exceptions if an object is not found by a unique identifier, we probably should fix HibernateProviderDAO
just to understand, do you mean:
- if one wants to get an object via its PK than I must know the exact key and thus if it does not exist is an exceptional event
- if I for example want to get an object via lets say partial of its name (not PK) I do not the exact name, and it does not exist, its not an exceptional event and therefore returning null is justified to show there is no result
Yes, that’s what I mean. This kind of search is never performed by the user, it’s internal (WS, form).
Yes, in this case it’s a search performed by a final user. It should return an empty list, not null.
thanks! I do see value in this approach.
and yes @dkayiwa we could of course change all the other DAO implementations, but this would have a bigger impact. ObjectNotFoundException thrown by getOurDomainObject() would need to be caught somewhere and Im sure it isnt at the moment, so I assume UIs would show the exception.
In the past, I used session.load() for to increase performance since session.load() is not hitting database directly and session.get() does. When I use session.load() and if there is no row found, I would handle the ObjecNotFoundException gracefully. I am not sure in this case we need to worry about performance or not. However, performance is always and should be in developer’s mind. Because, we will have to deal with the performance later anyhow.
@oslynn is the performance difference noticeable?
Yes @dkayiwa, it is noticeable. However, it is acceptable for now. All cloud software as OpenMRS need to address performance. However, it is a very BIG task to tackle for now and it is an on going task. It depends on many components. We can only do our best. Perhaps, we can address it later. Do you know where I can find the link to OpemMRS performance goal page?
Some years back, we had this: https://wiki.openmrs.org/display/RES/Top+Ten+List+for+1.8+Performance+Improvements Then there was this module: https://wiki.openmrs.org/display/docs/System+Performance+and+Utilization+Module We have also had conversations here update performance tests in openmrs-core and here Performance testing of RA
Thank you sir. I will look at all the links as soon as I have a chance.
If the instance is already in session get does not hit the database. It’s the first level cache.
I agree. If the persistent is not in session, it should be retrieved from database (this will happen just once in the current session). I believe in the 99% of use cases this is fine.
@lluismf, thank you very much for pointed out the first level cache of get(). In openmrs case, using get() should be fine and won’t cause performance issue, then