comments inline ...
David M Johnson wrote:
On Mar 26, 2006, at 8:34 PM, Allen Gilliland wrote:
I have updated my backend refactoring proposal to include a bit more
implementation details.
http://rollerweblogger.org/wiki/Wiki.jsp?page=Proposal_BackendRefactorings
I have basically listed all the manager classes which use the database
and then listed each method of those classes which involves writing data.
I still stand behind my proposal that we should remove all persistence
specific logic from existing outside the business/presentation layer.
I am planning to begin work on this for April and hopefully will get
it completed for Roller 2.3
The more I think about this idea, the more I like it. I'd like to see
the Roller backend be a self-contained API and this appears to be the
way to achieve that.
cool. glad to hear my powers of persuasion aren't totally defunct ;)
But I'm still opposed to doing this work in the trunk. It involves
dramatic sweeping changes to the code base and requires that almost
every Struts action be rewritten. It should done done in a separate
branch and not merged back until it's been tested an evaluated.
yep. i think that's a good idea as well.
Plus, I've got a couple questions about the plan:
1) You propose to remove the setUser() and getUser() methods from the
Roller interface. What is the plan for determining that the current user
has permission to perform an operation?
ah. i didn't address this specifically anywhere in the proposal, but i
believe there are a couple problems with trying to do permissions
checking the way it is now. my feeling is that this type of check
should not be happening at the persistence layer because the persistence
layer has no way of forcing proper authentication.
right now the system is basically trying to force permission checks on
access to java methods, and i think that is a little tough to do. i
think it's better to leave the persistence layer open to anyone who has
access to the java code and instead expect that our struts actions are
ensuring proper permissions. so instead of trying to enforce
permissions on java methods we are enforcing permissions on access to
various struts actions.
also, we don't really have the right code in place to effectively
enforce permissions on persistence methods for a couple reasons.
1. we never force the appropriate user to be set before a given method
is called. there is nothing in the code preventing someone from just
always setting the user to UserData.SYSTEM_USER. and the code that does
the permissions checking just assumes that the set user is correct.
2. the user can be changed at any time and isn't required to stay the
same once it is set. so even if the code does properly set the user it
can just be overridden. the same way it happens in the
PropertiesManagerImpl.init() method.
so we would be pushing the logic outside the persistence layer and
expecting someone else to do authentication/authorization, not the
persistence layer.
2) You mention that some operation are "single step persistence
operations which do not require special transaction considerations."
What does that mean in practice? Does each of these operations do it's
own begin() and then end with a commit() or rollback()? Will these
single step operations be called from the multi-step operations?
this is ultimately up to the implementor of the persistence strategy.
they can do things however they want, as long as they ensure that a call
to any method that writes data is properly transacted.
but yes, i imagine that a singe step operation like
saveWeblogComment(comment) would just begin, then commit/rollback.
speaking specifically about Hibernate, note that calling begin() will
open a new transaction if needed, or join the current transaction if one
is already open. after a commit or rollback a new transaction can begin
if needed, but this will all happen within the same Session.
3) What is the plan for unit testing the changes to the Struts Actions
and other presentation classes?
well, most of our unit tests should still work fine, we just need to rip
out the methods I mentioned in the proposal. some of the unit tests
will have to be rewritten/updated though.
4) How did you come up with the list of changes? I assume you reviewed
each of the Struts actions. Did you also review the MetaWeblog API and
Atom protocol implementations?
I built the list initially from our current Manager interfaces and then
modified based on struts actions. I didn't look extensively at the
MetaWeblog API or Atom protocol.
-- Allen
- Dave
-- Allen
Allen Gilliland wrote:
Anil Gangolli wrote:
Allen:
Your suggested pattern is workable, but you'll need to make begin()
and commit() conditional and smart about transaction demarcation; it
is a common pattern to conditionally begin a transaction only if
there isn't one on the current thread, and you'll find that in many
infrastructures (see my comments on Section 2 in the design proposal).
I agree with this and believe it's fairly easy to do.
What happens if you don't have that logic? You'll find that the
operations that you choose to make transactional can't be easily
composed, which is a very awkward property to have. If you ever
need to compose two operations, you would need to write a third
method that includes all of the operations of the originals, and not
just calling the two original methods, but copying their innards. I
actually went through a period of trying to code apps this way in
the 90's before infrastructures like EJB started to normalize the
conditional demarcation patterns in Java. Lesson learned.
You are correct, that is absolutely a possible ramification and we
could end up making it more difficult to properly wrap some steps
together in a transaction. However, I would also argue that the
reverse scenerio is true for our current setup using more open
transaction demarcation. Allowing business/presentation code to
control transaction demarcation allows for transactions to become
overly inclusive and actually end up including more operations than
they should.
The patterns for transaction demarcation in the dispatch layers
involve three parts: (1) demarcation for requests on a per-request
basis in a Filter or some other centralized dispatch point (e.g. in
Axis or Struts apps in extensions of these servlets wrapping the
entry points) (2) for scheduled tasks, wrapping the run() method in
a base class (3) providing a mechanism to get an independent
transaction for individual units of work at smaller granularities
(REQUIRES_NEW style semantics), which I expect would be used very
rarely in Roller. Patterns like this work well for two-tier web
apps like Roller where the persistence container and the webapp
container are the same. They don't work (or require much stronger
transaction infrastructure) if you separate the webapp container
from the persistence container which does not apply in Roller's case.
In order to get a better grasp on some Hibernate details I am
currently skimming through "Hibernate in Action" and they say ...
"We merely observe that, in our opinion, overengineering has been
endemic in the Java community, and overly ambitious multilayered
architectures have significanly contributed to the cose of Java
development and to the perceived complexity of J2EE. On the other
hand, we do agree that a dedicated persistence layer is a sensible
choice for most applications and that persistence-related code
shouldn't be moxed with business logic or presentation." (page 296)
In their examples for designing a layered application (webapp
specifically) they do not provide transaction demarcation outside of
the DAO layer. Of course this is only a single opinion and does not
mean it's what's best for Roller, but I would say that my opinions
are more in line with the sentiments from the quote above.
Speaking more specifically about applying transaction demarcation at
points in the dispatch layer ... I think I still see this as allowing
persistence logic into the business/presentation layer. The other
reason why this makes me nervous is that I think it creates some
potential for two situations ...
1. catching a persistence exception too late. if the transaction
demarcation doesn't actually commit the transaction data until a
while after the app thinks the data has been written then we may end
up putting ourselves in a situation where we catch an exception after
we have already commited some of the response and are no longer in a
situation where we can respond appropriately. This seems especially
true if we consider each request/response cycle as a single
transaction and only flush/commit the data in a filter as we exit the
app.
2. commiting data changes we don't want, specifically users being bad
with their templates. I suppose our wrapper objects may take care of
the potential problem with users making changes to their data that we
don't want, but there may also be an issue of app code doing the same
thing. It has always seemed ideal to me to only commit changes
specifically when needed. If a filter simply commits data at the end
of the request/response cycle there is no guarantee that someone
hasn't modified an object that is not meant to be saved back to the
database.
Like your proposal, the dispatch-level patterns remove begin() and
commit() from app code, and we should end up with only a few places
that call them.
I like the idea of the dispatch-level pattern much more than our
current pattern. After I spend a bit more time on the proposal and
get things more flushed out I think I'll have a better idea of which
approaches make the most sense.
-- Allen
--a.
Allen Gilliland wrote:
On Thu, 2006-03-09 at 14:21, David M Johnson wrote:
On Mar 9, 2006, at 5:02 PM, Allen Gilliland wrote:
i feel like we are still a little disconnected here. my
approach will not alter anything about our ability to use the
open session in view pattern. the only difference i see is the
difference between these 2 code blocks ...
old way:
begin();
website.store();
commit();
new way:
WeblogManager wmgr = roller.getWeblogManager();
wmgr.saveWebsite(website);
But what if saving a website is just one step in a multi-part
transaction? OK, scratch that. If you start by doing the analysis
and designing one of the new Manager interfaces, I think we'll
have a much better understanding of this proposed refactoring.
i'll just give the short answer now. usually things that are
multipart operations are still part of a single logical operation.
in fact, we already do this in some places. take a look at the
UserManager.createWebsite() method. that is a logical method which
wraps a series of persistence operations. so the difference would
be ...
old way (in struts action or servlet):
begin();
// setup website
website.store();
// setup template
template.store();
// setup category
category.store();
// etc etc
commit();
new way (logical method in manager class):
UserManager umgr = roller.getUserManager();
umgr.createWebsite(newWebsite, possible, other, data);
when i use the new way i only care wether or not the operation
succeeded and i don't have to think about the persistence
implications because they are hidden from me. now if it turns out
that we have way too many transactions that would need their own
logical method in a manager class then maybe this approach won't
work, but i believe that in most request/response cycles the
operations are pretty isolated and simple. in most of them you are
only creating/updating/deleting a single object at a time.
-- Allen
- Dave
