I have written this utility class called DetachedObject which I want to run by the experts here:



The idea here is that I'm going to store the DetachedObject in the HttpSession of my webapp. When a request comes back in, I'd like to reliably be able to reattach the object to the current session (I'm using open-session-in-view). I'm implementing a custom "persistence strategy" in Tapestry. So, for persistent page/component properties of type "entity" I'd use this class to store the object into the session.

1. I always want to store the object "as-is" because I want to account for optimistically locked (versioned) objects. If I just store the identity of the object (entity name and identifier), then I basically circumvent the version checking logic, because I'd be reading the latest version out of the db. I realize that when the occasion does arise when a versioned object has been modified since I last loaded it, the call to merge() will fail with a StaleStateException, but that's what I want. I want to know that. Does this sound about right to you folks? I could make sure my HTML forms update the version property, but I read somewhere that you're really not supposed to manually set the version property.

2. If the object is "new", I don't want to reattach it at all (as it wasn't persistent in the first place). I am using the contains() method to see if something is persistent. Is this a good way to do it? Does anyone see anything else that's flawed with this approach? Is there a better way to see if an object is "new"? Because, it could have been loaded in another Session, but it's still persistent. I guess I could ask for the identity and catch the exception that's thrown when it doesn't have one (I think I've done that before), but I hate using exceptions for logic.

Sorry for the long post.

Hibernate version: 3.1

Looks reasonable, except for the early calculation of reattachRequired. What if someone writes code like this:Obviously a very contrived example, but according to Murphy's Law (and it is almost St. Patrick's day, so that law is currently in force), something like this will happen if you let it.

Not quite. In your example, reattach would be required. What I really need to ask is "is this object persistent in the database?" It doesn't really, necessarily have to be associated with the current session, but it does have an identifier. I guess I could use the ClassMetaData object to find that out, but aren't we not supposed to use that?[/quote]

That's right, reattach would be required, but your utility class wouldn't reattach it because it calculated the value of reattachRequired too soon.

If you just want to find out if an object's identifier isn't the unsaved-value, can't you just go "if (obj.getId() != null)" or whatever? Why would you create an entire utility class to do that in some roundabout class-insensitive way?

I presume that you're aware of the various states an instance of a mapped class could be in. If the id isn't null (or equal to unsaved-value, whatever you've set that up to be), that means that the object isn't transient. If session.contains(obj) is false, then the object is detached. Your utility class does not handle transient objects at all, just attached and detached objects.

Are you also aware that session.lock(obj, LockMode.NONE) does some of what you're describing? It doesn't change the values of the object or its DB row (merge updates the DB during the next flush, if required). I don't think you want to use it, I just wanted to make sure that you're aware of its existence.

Yeah, I knew about the lock() method, but you're right, I don't think I can use it reliably. I can't use update() either, because I'm not positive that the object hasn't been loaded before. And, I want to come up with a reusable way to do this. It's going to be a reusable library, not specific to my domain model (my entity classes do have an "id" property that I use). As for my code, why wouldn't it think that the a reattach was required? You loaded "obj" in the session, so it would belong to the session. So, in the constructor reattachRequired would be true. So, do you think I should go with the ClassMetaData.getIdentifier() method to check if my identifier is not null? Do you think that's the most reusable way to do it?

Oops, another public error on my part. My sample code should have looked like this:In my original code, my comment was right but my code was wrong. The worry would be that an object that does require a merge wouldn't get one: the new sample code illustrates this.

This case can be avoided by publishing development standards and ensuring that people who use your utility class adhere to them. Vis.: only pass transient or attached objects to DetachedObject's constructor, don't pass in detached objects.

An alternative (additional?) approach that I briefly played with (and still use for one particular hierarchy of classes in one of my apps) is to define a simple interface with nothing but a getId() (or (isTransient()?) method in it, then require that all persistable objects implement it. That would mean you wouldn't need to use class metadata. If you then checked for getId() != null (or isTransient() == false) you'd have a fuller picture of the state of the object. Using both session.contains(obj) and obj.isTransient() tells you exactly what state hibernate thinks the object is in.

If you liked that approach, I suggest the isTransient() method rather than getId(). Checking a value for null (meaning "being unset") to decide what to do is what my old compsci prof called "logcial coding", and he rated that up with tightly coupled code as one of the biggest bugbears in software development.

First of all, thank you for engaging in this conversation with me. I really appreciate it. As for requiring that my "clients" adhere to my coding standards, that's a bit too heavy-handed for what I'm trying to do. I'm working on the Trails Framework (www.trailsframework.org) project and we don't want to require that our clients have to extend a class or implement an interface. Of course, we could achieve this with an aspect, but I am trying to make sure our clients don't have to use aspects to do what they need to do (aspects clutter up the build environment IMHO). What I'm trying to come up with is an unobtrusive solution to this common problem. I know it's not easy; that's why I'm asking the experts! :-)

Your code will work most of the time, but the 90:10 rule will bite you: the 10% of cases (non-transient detached objects) will take up 90% of your development time.

You said before that your classes do have an id property. Can you use that in your utility class? If you can, then you can handle all three states that persistable objects can be in: transient (id is unset/equal to unsaved-value), detached (session.contains = false) and attached (session.contains = true).

Yes, my classes do have an "id" property. But, what I'm trying to create (as part of the Trails Framework) is a reusable Tapestry property persistence strategy. So, I need to come up with a reusable way to determine what state the object is in. [/b]

How about changing the utility class to this?

That won't work either. Basically, I'm going to be putting the DetachedObject objects into the HttpSession rather than putting the actual object in there (because the DetachedObject will have some context of what was going on when the object went into the session). The DetachedObject class is not something that client code will use. It's an implementation detail. I will not have the id available when I'm reading the DetachedObject out of the session.

Then you could allow the user of DetachedObject, who presumably does know about implementation details, to decide whether or not the object should be reattached later. I.e. don't calculate reattachRequired, instead make it a parameter to the constructor.

For now, this is what I have: