You are viewing a plain text version of this content. The canonical link for it is here.
Posted to user@struts.apache.org by Eric D Nielsen <ni...@MIT.EDU> on 2008/03/25 15:29:20 UTC
S2: Actions/DAO interaction getting messy...
I'm starting to get some rather stinky code in one of my projects and wanted to
ask for some advice. (
Its a Struts2/Spring2/JPA(Hibernate) based project. I'm using a slightly
modified version of the Generic DAO pattern shown in the Java persistence with
Hibernate book and/or the IBM ThoughtWorks very similar example. (Modified to
allow Spring Based Injection of the JPA EntityManger, while falling back to a
native Hibernate session inside the DAOs to allow a few more optimizations).
So its basically
Business Objects (POJOs) <---1:1---> DAOs
which is a relatively normal pattern I beleive.
Now I'm working on an Action that creates an object that contains lots of
references to other objects. A bug tracker would be a reasonable facsimile of
my domain object -- when creating a bug, you select a severity, a project, a
module, a found in version, possibly you're assigning it to someone, etc. Most
of these options come from a drop-down on the view page, however they are all
backed by their own domain object -- ie its not just id/string display name
lookup-table backing the drop down.
So the form submits a whole suite of IDs or String natural keys and I need to
build an Defect/Bug and then persist it. In building the Defect I need to
convert all the keys to domain objects. This is where things begin to fall
apart. To covert the keys to domain object seems to require injecting all the
subordinate object DAOs into the Action, its not a big deal but its starting to
feel like the subordinate DAOs are taking over the action. Furthermore, its
slightly annoying from a performance stand-point that I need to retrieve all
the objects from the DB, just to set a foreign key -- I will never be updating
the details on the subordinate object from the master only changing which one
I'm linked to. [As a side issue: this is a place where the Get (returning a
proxy/lazy load thunk with only the ID set without hitting the DB) versus Load
could be useful, but I've never seen any of the generic DAO approaches expose
that level of control in their API -- does anyone know why?]
Here's an example (not from live code, please ignore any minor typos)
representing the current state of application and test code. Afterwards I'll
list a few of the approaches I've considered for cleaning it up:
AddBug.java
code:
public class AddBug extends ActionSupport {
private BugDAO bugService;
private UserDAO userService;
private ProjectDAO projectService;
private ModuleDAO moduleService;
private VersionDAO versionService;
private String submitterUsername;
private String assigneeUsername;
private Long projectID;
private Long moduleID;
private List<Long> affectedVersionIDs;
public String execute() {
Bug bug = new Bug();
// The following lines are what feel unclean to me
bug.setSubmitter(userService.findByUsername(getSubmitterUsername()));
bug.setAssignee(userService.findByUsername(getAssigneeUsername()));
bug.setProject(projectService.find(getProjectID()));
bug.setModule(moduleService.find(getModuleID()));
for( Long versionID : getAffectedVersionIDs()) {
bug.addAffectedVersion(versionService.find(versionID));
}
bugDAO.persist(bug);
return SUCCESS;
}
// This block of setters also seems to be wrong... Only the first one
// should really be required by this action I think....
public void setBugService(final BugDAO bs) {bugService=bs;}
public void setUserService(final UserDAO us) {userService=us;}
public void setProjectService(final ProjectDAO ps) {projectService=ps;}
public void setModuleService(final ModuleDAO ms) {moduleService=ms;}
public void setVersionService(final VersionDAO vs) {versionService=vs;}
public void setSubmitterUsername(final String username)
{submitterUsername=username;}
public void setAssigneeUsername(final String username)
{assigneeUsername=username;}
public void setProjectID(final Long id) {projectID=id;}
public void setModuleID(final Long id) {moduleID=id;}
public void setAffectedVersionIDs(final List<Long> ids)
{affectedVersionIDs=ids;}
public String getSubmitterUsername() {return submitterUsername;}
public String getAssigneeUsername() {return assigneeUsername;}
public Long getProjectID() {return projectID;}
public Long getModuleID() {return moduleID;}
public List<Long> getAffectedVersionIDs() {return affectedVersionIDs;}
}
TestAddBug.java
(Sorry this is purely from memory so I know I'm missing some of my
infrastructure)
code:
....
public void testAddBug() {
String submitterUsername="TestUser";
String assigneeUsername="TestVictim";
User submitter=new User(submitterUsername);
User assignee=new User(assigneeUsername);
UserDAO userService = createMock(UserDAO.class);
expect(userService.findByUsername(submitterUsername)).andReturn(submitter);
expect(userService.findByUsername(assigneeUsername)).andReturn(assignee);
Long projectID =1L;
Project project = new Project(projectID,"Some Projectname");
ProjectDAO projectService = createMock(ProjectDAO.class);
expect(projectService.find(projectID)).andReturn(project);
Long moduleID=1L;
Module module = new Module(moduleID,"Some ModuleName");
ModuleDAO moduleService = createMock(ModuleDAO.class);
expect(moduleService.find(moduleID)).andReturn(module);
Long versionOneID=1L;
Long versionTwoID=2L;
Version versionOne = new Version(versionOneID,"A Version");
Version versionTwo = new Version(vertionTwoID,"Another Version");
List<Long> versionIDs = new ArrayList<Long>();
List<Version> versions = new ArrayList<Version>();
versionIDs.add(versionOneID);
versionIDs.add(versionTwoID);
versions.add(versionOne);
versions.add(versionTwo);
VersionDAO = versionService = createMock(VersionDAO.class);
expect(versionService.buildVersionlist(versionIDs)).andReturn(versions);
Bug bug = new Bug();
BugDAO bugService = createMock(BugDAO.class);
expect(bugService.persist(bug));
replay(userSerive);
replay(projectService);
replay(moduleService);
replay(versionService);
replay(bugService);
AddBug action = new AddBugAction();
action.setBugService(bugService);
action.setProjectService(projectService);
action.setModuleService(moduleService);
action.setVersionService(versionService);
action.setUserService(userService);
// In reality this would probably be pulled from the session map, but...
action.setSubmitterUsername(submitterUsername);
action.setAssigneeUsername(assigneeUsername);
action.setProjectID(projectID);
action.setModuleID(moduleID);
action.setAffectedVersionIDs(versionIDs);
assertEquals("success",action.execute());
verify(bugService);
verify(userService);
verify(projectService);
verify(moduleService);
verify(versionService);
}
...
OK. Now that you've seen the mess... I've thought of two ways to clean it up:
1) Introduce a business tier between the domain objects and the DAOs. Expose a
buildBug call in this layer that takes in the keys and handles the promotion to
subordinate domain objects. This business tier would need to have all the DAO
injected into it. Actions would still have their most relevant DAO(s) injects,
plus the business tier relevant objects. Testing the business tier would still
require the mess of mocks, but at least the action only needs to mock the
single call on the business tier. However I don't think this approach would
play too well with migrating to a Model-Driven strategy in the future as the
domain objects can't accept the bare keys. I'm also having a hard time figuring
out what type of methods would end up in this layer -- it seems like mostly it
will be these kinds of initial builders... Most of other use cases are either
correctly cascaded/handled by Hibernate (updates/deletes) and the DAOs or they
belong on the POJOs (real business logic)... Leading to a very anemic layer,
which doesn't seem worth the added complexity...
2) Setup a series of type converters to promote the keys to objects on the
incoming HTTP request end of things. The type converters would need access to
the DAOs. Testing them should be rather simple as each singular one is simple.
Testing the action no longer requires mocking the subordinate objects and I can
just use POJOs there. I believe this approach would work with the Model-Driven
idea, but it feels a little odd to me to mark up the domain object with
HTTP-specific details.... then again the domain object is already annotated
with Hibernate/JPA annotations so its not like its really 'pure'.. I've seen
some talk on the mailing list of this approach, but it seems to have some
"black holes" regarding converting back to string for redsplay at times.
Comments, alternate approaches?
---------------------------------------------------------------------
To unsubscribe, e-mail: user-unsubscribe@struts.apache.org
For additional commands, e-mail: user-help@struts.apache.org
Re: S2: Actions/DAO interaction getting messy...
Posted by Laurie Harper <la...@holoweb.net>.
Eric D Nielsen wrote:
> I'm starting to get some rather stinky code in one of my projects and wanted to
> ask for some advice. (
>
> Its a Struts2/Spring2/JPA(Hibernate) based project. I'm using a slightly
> modified version of the Generic DAO pattern shown in the Java persistence with
> Hibernate book and/or the IBM ThoughtWorks very similar example. (Modified to
> allow Spring Based Injection of the JPA EntityManger, while falling back to a
> native Hibernate session inside the DAOs to allow a few more optimizations).
>
> So its basically
> Business Objects (POJOs) <---1:1---> DAOs
> which is a relatively normal pattern I beleive.
>
> Now I'm working on an Action that creates an object that contains lots of
> references to other objects. A bug tracker would be a reasonable facsimile of
> my domain object -- when creating a bug, you select a severity, a project, a
> module, a found in version, possibly you're assigning it to someone, etc. Most
> of these options come from a drop-down on the view page, however they are all
> backed by their own domain object -- ie its not just id/string display name
> lookup-table backing the drop down.
>
> So the form submits a whole suite of IDs or String natural keys and I need to
> build an Defect/Bug and then persist it. In building the Defect I need to
> convert all the keys to domain objects. This is where things begin to fall
> apart. To covert the keys to domain object seems to require injecting all the
> subordinate object DAOs into the Action, its not a big deal but its starting to
> feel like the subordinate DAOs are taking over the action. Furthermore, its
> slightly annoying from a performance stand-point that I need to retrieve all
> the objects from the DB, just to set a foreign key -- I will never be updating
> the details on the subordinate object from the master only changing which one
> I'm linked to. [As a side issue: this is a place where the Get (returning a
> proxy/lazy load thunk with only the ID set without hitting the DB) versus Load
> could be useful, but I've never seen any of the generic DAO approaches expose
> that level of control in their API -- does anyone know why?]
>
> Here's an example (not from live code, please ignore any minor typos)
> representing the current state of application and test code. Afterwards I'll
> list a few of the approaches I've considered for cleaning it up:
> AddBug.java
>
> code:
>
>
>
> public class AddBug extends ActionSupport {
> private BugDAO bugService;
> private UserDAO userService;
> private ProjectDAO projectService;
> private ModuleDAO moduleService;
> private VersionDAO versionService;
>
> private String submitterUsername;
> private String assigneeUsername;
> private Long projectID;
> private Long moduleID;
> private List<Long> affectedVersionIDs;
>
> public String execute() {
> Bug bug = new Bug();
> // The following lines are what feel unclean to me
> bug.setSubmitter(userService.findByUsername(getSubmitterUsername()));
> bug.setAssignee(userService.findByUsername(getAssigneeUsername()));
> bug.setProject(projectService.find(getProjectID()));
> bug.setModule(moduleService.find(getModuleID()));
> for( Long versionID : getAffectedVersionIDs()) {
> bug.addAffectedVersion(versionService.find(versionID));
> }
> bugDAO.persist(bug);
> return SUCCESS;
> }
>
> // This block of setters also seems to be wrong... Only the first one
> // should really be required by this action I think....
> public void setBugService(final BugDAO bs) {bugService=bs;}
> public void setUserService(final UserDAO us) {userService=us;}
> public void setProjectService(final ProjectDAO ps) {projectService=ps;}
> public void setModuleService(final ModuleDAO ms) {moduleService=ms;}
> public void setVersionService(final VersionDAO vs) {versionService=vs;}
>
> public void setSubmitterUsername(final String username)
> {submitterUsername=username;}
> public void setAssigneeUsername(final String username)
> {assigneeUsername=username;}
> public void setProjectID(final Long id) {projectID=id;}
> public void setModuleID(final Long id) {moduleID=id;}
> public void setAffectedVersionIDs(final List<Long> ids)
> {affectedVersionIDs=ids;}
>
> public String getSubmitterUsername() {return submitterUsername;}
> public String getAssigneeUsername() {return assigneeUsername;}
> public Long getProjectID() {return projectID;}
> public Long getModuleID() {return moduleID;}
> public List<Long> getAffectedVersionIDs() {return affectedVersionIDs;}
> }
>
>
>
> TestAddBug.java
> (Sorry this is purely from memory so I know I'm missing some of my
> infrastructure)
>
> code:
>
>
> ....
> public void testAddBug() {
> String submitterUsername="TestUser";
> String assigneeUsername="TestVictim";
> User submitter=new User(submitterUsername);
> User assignee=new User(assigneeUsername);
> UserDAO userService = createMock(UserDAO.class);
>
> expect(userService.findByUsername(submitterUsername)).andReturn(submitter);
>
> expect(userService.findByUsername(assigneeUsername)).andReturn(assignee);
>
> Long projectID =1L;
> Project project = new Project(projectID,"Some Projectname");
> ProjectDAO projectService = createMock(ProjectDAO.class);
> expect(projectService.find(projectID)).andReturn(project);
>
> Long moduleID=1L;
> Module module = new Module(moduleID,"Some ModuleName");
> ModuleDAO moduleService = createMock(ModuleDAO.class);
> expect(moduleService.find(moduleID)).andReturn(module);
>
> Long versionOneID=1L;
> Long versionTwoID=2L;
> Version versionOne = new Version(versionOneID,"A Version");
> Version versionTwo = new Version(vertionTwoID,"Another Version");
> List<Long> versionIDs = new ArrayList<Long>();
> List<Version> versions = new ArrayList<Version>();
> versionIDs.add(versionOneID);
> versionIDs.add(versionTwoID);
> versions.add(versionOne);
> versions.add(versionTwo);
> VersionDAO = versionService = createMock(VersionDAO.class);
> expect(versionService.buildVersionlist(versionIDs)).andReturn(versions);
>
> Bug bug = new Bug();
> BugDAO bugService = createMock(BugDAO.class);
> expect(bugService.persist(bug));
> replay(userSerive);
> replay(projectService);
> replay(moduleService);
> replay(versionService);
> replay(bugService);
>
> AddBug action = new AddBugAction();
> action.setBugService(bugService);
> action.setProjectService(projectService);
> action.setModuleService(moduleService);
> action.setVersionService(versionService);
> action.setUserService(userService);
>
> // In reality this would probably be pulled from the session map, but...
> action.setSubmitterUsername(submitterUsername);
>
> action.setAssigneeUsername(assigneeUsername);
> action.setProjectID(projectID);
> action.setModuleID(moduleID);
> action.setAffectedVersionIDs(versionIDs);
>
> assertEquals("success",action.execute());
> verify(bugService);
> verify(userService);
> verify(projectService);
> verify(moduleService);
> verify(versionService);
> }
> ...
>
>
>
> OK. Now that you've seen the mess... I've thought of two ways to clean it up:
>
> 1) Introduce a business tier between the domain objects and the DAOs. Expose a
> buildBug call in this layer that takes in the keys and handles the promotion to
> subordinate domain objects. This business tier would need to have all the DAO
> injected into it. Actions would still have their most relevant DAO(s) injects,
> plus the business tier relevant objects. Testing the business tier would still
> require the mess of mocks, but at least the action only needs to mock the
> single call on the business tier. However I don't think this approach would
> play too well with migrating to a Model-Driven strategy in the future as the
> domain objects can't accept the bare keys. I'm also having a hard time figuring
> out what type of methods would end up in this layer -- it seems like mostly it
> will be these kinds of initial builders... Most of other use cases are either
> correctly cascaded/handled by Hibernate (updates/deletes) and the DAOs or they
> belong on the POJOs (real business logic)... Leading to a very anemic layer,
> which doesn't seem worth the added complexity...
>
> 2) Setup a series of type converters to promote the keys to objects on the
> incoming HTTP request end of things. The type converters would need access to
> the DAOs. Testing them should be rather simple as each singular one is simple.
> Testing the action no longer requires mocking the subordinate objects and I can
> just use POJOs there. I believe this approach would work with the Model-Driven
> idea, but it feels a little odd to me to mark up the domain object with
> HTTP-specific details.... then again the domain object is already annotated
> with Hibernate/JPA annotations so its not like its really 'pure'.. I've seen
> some talk on the mailing list of this approach, but it seems to have some
> "black holes" regarding converting back to string for redsplay at times.
>
> Comments, alternate approaches?
Adding a suite of type converters and a service layer would certainly
clean up your action code a lot. Your example action should shrink to
just a getBug()/setBug() method pair and a call to bugService.persist(),
more or less.
Your code seems to mix 'bugService' and 'bugDAO' freely, so it's worth
repeating the recommendation to introduce an intermediate service tier.
You bugService would take in an instance of Bug, fully populated
already. Therefore it's easy to unit test from there down. Your action
becomes trivial to test too, since it's doing very little once you have
all the data marshalling factored out into custom converters.
The final issue of database 'chattyness' should probably be ignored at
this stage; that's an performance/optimization issue, which you can
address later (once you've measured to determine if it's significant or
not) with the various caching strategies Hibernate provides for.
L.
---------------------------------------------------------------------
To unsubscribe, e-mail: user-unsubscribe@struts.apache.org
For additional commands, e-mail: user-help@struts.apache.org
Re: [struts] S2: Actions/DAO interaction getting messy...
Posted by Dale Newfield <Da...@Newfield.org>.
The other benefit of the DAO / Manager / Action layers is that you can
use spring to wire up the Manager/Service methods as the transaction
boundaries. Sets of changes you want to all succeed or fail atomically?
Put it in the manager. Managers are where business logic belongs,
DAO's where DB maintenance logic belongs, Actions are just one interface
into this, as you can expose managers as web services, too...
-Dale
---------------------------------------------------------------------
To unsubscribe, e-mail: user-unsubscribe@struts.apache.org
For additional commands, e-mail: user-help@struts.apache.org
Re: S2: Actions/DAO interaction getting messy...
Posted by Adam Hardy <ah...@cyberspaceroad.com>.
Eric D Nielsen on 25/03/08 14:29, wrote:
> Its a Struts2/Spring2/JPA(Hibernate) based project. I'm using a slightly
> modified version of the Generic DAO pattern shown in the Java persistence with
> Hibernate book and/or the IBM ThoughtWorks very similar example. (Modified to
> allow Spring Based Injection of the JPA EntityManger, while falling back to a
> native Hibernate session inside the DAOs to allow a few more optimizations).
>
> So its basically
> Business Objects (POJOs) <---1:1---> DAOs
> which is a relatively normal pattern I beleive.
Is it normal with that Generic DAO pattern to name the DAOs 'services'? In the
Domain-Driven-Design paradigm that I generally follow, the services are objects
which carry out operations that you don't want to specifically assign to one
domain object.
> [As a side issue: this is a place where the Get (returning a
> proxy/lazy load thunk with only the ID set without hitting the DB) versus Load
> could be useful, but I've never seen any of the generic DAO approaches expose
> that level of control in their API -- does anyone know why?]
that's a pure Hibernate thing, I think, not a JPA distinction.
> just use POJOs there. I believe this approach would work with the Model-Driven
> idea, but it feels a little odd to me to mark up the domain object with
> HTTP-specific details....
Yes it would do, but what do you mean by 'mark up the domain object with
HTTP-specific details'? I don't think you have to touch the domain objects to
code the re-construction of your incoming Bug POJO.
Regards
Adam
---------------------------------------------------------------------
To unsubscribe, e-mail: user-unsubscribe@struts.apache.org
For additional commands, e-mail: user-help@struts.apache.org