Announcement Announcement Module
Collapse
No announcement yet.
Service Layer Exceptions Page Title Module
Move Remove Collapse
X
Conversation Detail Module
Collapse
  • Filter
  • Time
  • Show
Clear All
new posts

  • #31
    Originally posted by ramoq View Post
    - So yes, essentially I DO NOT want to recover from this error. No doubt there.

    - However, I would like to prompt the caller of this service that the email used already exists with a EmailExistsException (RTE of course ) or something along those lines.

    - This would then allow the caller to handle this in a specific fashion. Ie. prompt the user to re-enter the password with a specific msg. "please enter a different email, this one is in use.."

    - I guess flush() is out of the question.

    - However, since I was keeping my DAO layer limited to very basic CRUD operations( save(), update(), delete() findByEmail() etc). This was the reason I was opting for the hacky solution I posted. Unless you guys think this is a bad design for DAO layers (containing very basic crud operations only)

    - Should have a createUser() method in my DAO and wrap that with a try/catch in my service? Obviously the createUser DAO call would run in it's own transaction.

    - what would a solution look like in terms of what I listed? Avoiding aspects if possible.
    Very interesting topic this.

    Would be interesting in hearing their opinion on attempting to catch the exception in the service layer.

    I think wrapping your DAO call in a transaction should be avoided.
    eg this particular DAO call(creatUser) could be used by another service method(another api maybe). if this new service method fails for another unrelated reason all changes made by the new service will be rolled back(assuming it is also transactionally scoped) but createUser call will not.

    This will eventually lead to inconsistent data or orphaned data.

    Very good discussion by the way

    Comment


    • #32
      Originally posted by ramoq View Post
      - So yes, essentially I DO NOT want to recover from this error. No doubt there.

      - However, I would like to prompt the caller of this service that the email used already exists with a EmailExistsException (RTE of course ) or something along those lines.

      - This would then allow the caller to handle this in a specific fashion. Ie. prompt the user to re-enter the password with a specific msg. "please enter a different email, this one is in use.."

      - I guess flush() is out of the question.

      - However, since I was keeping my DAO layer limited to very basic CRUD operations( save(), update(), delete() findByEmail() etc). This was the reason I was opting for the hacky solution I posted. Unless you guys think this is a bad design for DAO layers (containing very basic crud operations only)

      - Should have a createUser() method in my DAO and wrap that with a try/catch in my service? Obviously the createUser DAO call would run in it's own transaction.

      - what would a solution look like in terms of what I listed? Avoiding aspects if possible.
      I am a bit confused about your example... What are you trying to do? Create a user? What does it have to do with emails? Do you mean an "email address", as the user identifier? Or are you talking about some confirmation email being sent out once the user is successfully created? If the answer is the former, than EmailExistsException is a very confusing misnomer, in the first place. If the latter is the case, you probably should issue the confirmation email in a separate transaction that would not affect the creation of the user.

      Assuming that your "email" means "the user ID"... Here's what I would do, roughly:

      Service:
      Code:
      public class UserServiceImpl implements UserService {
          
          private UserDao dao;          // reference to the data access object used by the service
          
      
          /**
           * Sets the reference to the DAO implementation for this service.
           * @param theDao dao instance
           */
          @Required
          public void setDao(UserDao theDao) {
              dao = theDao;
          }
          
      
          /**
           * Creates a new user from the given User object and returns the updated object for the new user.
           * @param theDao dao instance
           * @return updated user object (perhaps, with the new unique ID generated, or/and additional status info, etc.)
           */
          @Transactional
          public User createUser(User user) {
            
          // any business logic (if needed)
          return dao.insertNewUser(user); // or, perform add'l logic and then return
          }
           
          ... other methods here
      
      }
      The DAO:

      Code:
      public class UserDaoImpl extends [whatever Spring DAO support you are using] implements UserDao {
      
         ...
         
         @Transactional
         public User insertNewUser(User user) {
            
             User existingUser = findExistingUser(user.getEmailAddress());
             if (existingUser == null) {
                 // perform an insert, perhaps get new user ID, and return updated user object
             } else {
                  throw new DuplicateUserException("A user with email address " + existingUser.getEmailAddress + " already exists; Existing User database ID = " + existingUser.getid()); // this message will only be displayed for debugging in log file (the resolver will log it), not in the UI. You will map the exception to the appropriate UI view/message
             }
          
      
         }
         ...
         
        // this method may also be public if you find that useful
         private User findExistingUser(String userEmailAddress) {
             // lookup user by email address
         }
      
      }
      This is what I would prefer. Nice and simple. No try/catches, just a very legitimate sanity check for an important business condition and one throw. I'd let uncaught constraint violations propagate to the generic handler. (Needless to say, DuplicateUserException is a RTE because no one needs to be aware of it except your designated resolver on the presentation tier side.) Since DuplicateUserException is part of your User Domain (it should be!) - along with the User service/DAO it is perfectly legitimate for both DAO and presentation tier (that is aware of the User Domain) to be aware of this exception.

      Note that simply wrapping the whole DAO method in the try/catch assuming that ANY exception (or any constraint violation) would mean the fact that the user already exists - and not some other problem - is unreliable. So, you treat you very legitimate and expected business case very consciously and specifically - by checking exactly what you need to check. The rest would just mean a critical failure in the DA tier, and won't carry any additional meaning for your application. So you would just want it to freely propagate to the top handler/resolver, get logged with the full stack trace, and result in a lovely generic error page that does not scare the pants off the user.

      Also, keep in mind that if you nest transactional methods (e.g. the dao method is nested within the service method), by default, any exception within the wrapping method will roll back any successful transaction within the nested method - unless you specify otherwise in the Transactional annotation properties.

      Your User packages should look similar to this:

      com.yourcompany.user.domain (User, Role, etc., DuplicateUserException, SomeOtherUserException, etc.)
      com.yourcompany.user.service (UserService interface, helper classes, implementation, etc.)
      com.yourcompany.user.service.dao (DAO interface, implementation)

      Don't make a mistake of doing something like this:

      com.yourcompany.domain.user
      com.yourcompany.domain.order
      com.yourcompany.service.user
      etc.

      You don't want to bundle all your domain entities together in a monolithic module, and you don't want to do that with your services/persistence. package by functionality.

      HTH,
      C
      Last edited by constv; Mar 6th, 2009, 11:32 AM.

      Comment


      • #33
        I'm sorry, but your approach is wrong - really, it is a text book level error. There always exist a chance that record with the same key would be inserted between your check and subsequent insert. While in case of email-address identified user it is unlikely, it is not absolutely impossible. Such check-then-act sequence absolutely needs synchronization (in this case DB-level synchronization, that is to say lock. And it should be table-level (or custom in case of cooperative locking). Such lock is scalability killer.

        Not so elegant but working solution would be publishing of a flush method from DAO layer and calling it at before exiting from service layer before exit from service call. It would not have any performance impact (as flush would be called so and so at commit.

        As marking a DAO method as transactional as proposed by ramoq is not very wise -
        • if service method is transactional as well DAO method would join the same transaction and so flushes and deferred constrains checks would be delayed to the exit from service method, so you win nothing
        • If service method is not transactional and calls more then one DAO method - well it would not be atomic and can leave database in inconsistent state. Absolutely unacceptable choice.
        Regards,
        Oleksandr

        Originally posted by constv View Post
        I am a bit confused about your example... What are you trying to do? Create a user? What does it have to do with emails? Do you mean an "email address", as the user identifier? Or are you talking about some confirmation email being sent out once the user is successfully created? If the answer is the former, than EmailExistsException is a very confusing misnomer, in the first place. If the latter is the case, you probably should issue the confirmation email in a separate transaction that would not affect the creation of the user.

        Assuming that your "email" means "the user ID"... Here's what I would do, roughly:

        Service:
        Code:
        public class UserServiceImpl implements UserService {
            
            private UserDao dao;          // reference to the data access object used by the service
            
        
            /**
             * Sets the reference to the DAO implementation for this service.
             * @param theDao dao instance
             */
            @Required
            public void setDao(UserDao theDao) {
                dao = theDao;
            }
            
        
            /**
             * Creates a new user from the given User object and returns the updated object for the new user.
             * @param theDao dao instance
             * @return updated user object (perhaps, with the new unique ID generated, or/and additional status info, etc.)
             */
            @Transactional
            public User createUser(User user) {
              
            // any business logic (if needed)
            return dao.insertNewUser(user); // or, perform add'l logic and then return
            }
             
            ... other methods here
        
        }
        The DAO:

        Code:
        public class UserDaoImpl extends [whatever Spring DAO support you are using] implements UserDao {
        
           ...
           
           @Transactional
           public User insertNewUser(User user) {
              
               User existingUser = findExistingUser(user.getEmailAddress());
               if (existingUser == null) {
                   // perform an insert, perhaps get new user ID, and return updated user object
               } else {
                    throw new DuplicateUserException("A user with email address " + existingUser.getEmailAddress + " already exists; Existing User database ID = " + existingUser.getid()); // this message will only be displayed for debugging in lof file, not in the UI. You will map the exception to the appropriate UI view/message
               }
            
        
           }
           ...
           
          // this method may also be public if you find that useful
           private User findExistingUser(String userEmailAddress) {
               // lookup user by email address
           }
        
        }
        This is what I would prefer. Nice and simple. No try/catches, just a very legitimate check for an important business condition and one throw. (Needless to say, DuplicateUserException is a RTE because no one needs to be aware of it except your designated resolver on the presentation tier side.)

        Note that simply wrapping the whole DAO method in the try/catch assuming that ANY exception (or any constraint violation) would mean the fact that the user already exists - and not some other problem - is unreliable. So, you treat you very legitimate and expected business case very consciously and specifically - by checking exactly what you need to check. The rest would just mean a critical failure in the DA tier, and won't carry any additional meaning for your application. So you would just want it to freely propagate to the top handler/resolver, get logged with the full stack trace, and result in a lovely generic error page that does not scare the pants off the user.



        Also, keep in mind that if you nest transactional methods (e.g. the dao method is nested within the service method), by default, any exception within the wrapping method will roll back any successful transaction within the nested method - unless you specify otherwise in the Transactional annotation properties.

        HTH,
        C

        Comment


        • #34
          I'm sorry, but your approach is wrong - really, it is a text book level error. There always exist a chance that record with the same key would be inserted between your check and subsequent insert. While in case of email-address identified user it is unlikely, it is not absolutely impossible. Such check-then-act sequence absolutely needs synchronization (in this case DB-level synchronization, that is to say lock. And it should be table-level (or custom in case of cooperative locking). Such lock is scalability killer.
          Yes, you are correct about the synchronization issue, and such approach is a supplementary sanity check, not a guard from running into a constraint violation. No objection here. My point, the constraint violation exception still gets caught. And, usually, the exception itself will give you all the info you need in this case, e.g. wich indices are involved, etc. Nothing gets missed. The question is, how far do you want to go in identifying the cause of any such (perhaps, very marginal for such application) condition to reflect it in the UI. What is worth creating more complex and costly checks, and what is not. If you know that a record already exist, you don't even go there. Such check is cheap. If you check, it's not there, you attempt an insert, and, it turns out, somebody has just inserted the record with the same id, then you will catch the constraint violation. But you are right: this does not guarantee the prevention of constraint violations by any means, and additional solution is needed if required.

          As marking a DAO method as transactional as proposed by ramoq is not very wise
          I would say it does not have much purpose - if the DAO is used properly, i.e only from within a service method, which I always promote. As I have said many times, data access is just an implementation detail of the use case represented by the service API. So, DAOs should not be called directly by anyone other than services. And I agree that such operations should be atomic, preferably.

          Comment


          • #35
            - I see how constv's solution will work most of the time, since the chance is rare of the email (yes, used as an unique identifier) being inserted between the check and the insert.

            - So the ONLY full-proof solution to catch this particular violation(User exists exception) would involve a table lock(yikes!) or a flush()? (apparantly not soo costly? bcs its called before commit? )

            - So since the nested transactions( service -> dao ) would participate in one transaction, using the default setting, I'd have to call flush() from the service after the dao call? I guess this is the 100% solution to definitely catch this exception, bcs I do want to wrap it to trigger some unique behavior on the UI.

            - More so, this brings me to an interesting point: How do I distinguish what functionality goes into a dao vs. service. What is the rule? Bcs with basic crud operations in the dao, you can create any form of 'behaviour' from the service. However in this case we added that to the dao layer in particular ( insertNewUser() )?
            Last edited by ramoq; Mar 6th, 2009, 12:05 PM.

            Comment


            • #36
              Ok in case in Hibernate it is time between read and actual insert, which typically occurs not on call of the save() or persist(), but at flush() (triggered explicitly or implicitly).

              In your case explicit flush() at the end of the service method is not a big deal - it is disastrous for performance if you perform a lot of inserts (or updates or deletions) inside single transaction (e.g. in a loop) and flush after each of them - definitely not your case.

              Explicit flush() is not ONLY fool-proof solution, but simplest one.

              Regards,
              Oleksandr

              Originally posted by ramoq View Post
              - I see how constv's solution will work most of the time, since the chance is rare of the email (yes, used as an unique identifier) being inserted between the check and the insert.

              - So the ONLY full-proof solution to catch this particular violation(User exists exception) would involve a table lock(yikes!) or a flush()? (apparantly not soo costly? bcs its called before commit? )

              - So since the nested transactions( service -> dao ) would participate in one transaction, using the default setting, I'd have to call flush() from the service after the dao call? I guess this is the 100% solution to definitely catch this exception, bcs I do want to wrap it to trigger some unique behavior on the UI.
              Last edited by al0; Mar 6th, 2009, 01:30 PM.

              Comment


              • #37
                - More so, this brings me to an interesting point: How do I distinguish what functionality goes into a dao vs. service. What is the rule? Bcs with basic crud operations in the dao, you can create any form of 'behaviour' from the service. However in this case we added that to the dao layer in particular ( insertNewUser() )?
                Simple. As I have said before, the DAO layer is strictly for anything related to your particular data source and the way you communicate with that data source. All that stuff must be invisible to the service that uses it. It exposes the "what"s of the data operations, but not the "how"s and "where"s. Application business logic lives in the service. If that logic requires you to store or retrieve something to or from the data resource, it does it via the DAO, and does NOT depend at all on how the DAO is implemented.
                Last edited by constv; Mar 6th, 2009, 01:11 PM.

                Comment


                • #38
                  Hi Constantine,

                  Have I understood correctly that you had voluntarily recalled first part of your comment (regarding stored procedures, JDBC and data access encapsulation)?

                  Regards,
                  Oleksandr
                  Originally posted by constv View Post
                  Simple. As I have said before, the DAO layer is strictly for anything related to your particular data source and the way you communicate with that data source. All that stuff must be invisible to the service that uses it. It exposes the "what"s of the data operations, but not the "how"s and "where"s. Application business logic lives in the service. If that logic requires you to store or retrieve something to or from the data resource, it does it via the DAO, and does NOT depend at all on how the DAO is implemented.

                  Comment


                  • #39
                    Originally posted by al0 View Post
                    Hi Constantine,

                    Have I understood correctly that you had voluntarily recalled first part of your comment (regarding stored procedures, JDBC and data access encapsulation)?

                    Regards,
                    Oleksandr
                    Yeah, it was not really relevant because it would still assume that the commit would have already occurred by the time the second thread attempts to insert, unless we put some crafty stuff inside the procedure, which would make it worse.

                    Interesting... It's not easy to come up with an elegant architecturally sound solution for this, it seems. The only way to actually capture such racing conflict is upon the end of the service transaction - on the service method. However, that means we may have to make an assumption about the data access/data source implementation - in the service. Unless, as you have suggested, we put some "finalizing" method on the DAO interface that would not really expose what it does but just attempt to catch such situation and throw an appropriate exception. Neither of the approaches sits well with me. I wonder what other people think.

                    I tend to think that in cases like that it may be reasonable to consider a sensible compromise. Not that I like this very much myself, but consider this... We do just a basic sanity check first - look up an existing user. If not found, proceed with an insert attempt. If a constraint violation exception occurs - specifically, perhaps, allow the user to retry once? (Ughhh...) The second time around, the sanity check should give a clear indication that the user already exists. What do you think? (In most cases, such stuff will not be needed, only in specific cases like ramoq's.) The ugly part about this, of course, is that the first time around the less clear message would still have to be shown to the user...
                    Last edited by constv; Mar 6th, 2009, 02:16 PM.

                    Comment


                    • #40
                      As I further examine constv's solution. The check for the email being unique (as an identifier) can be interpreted as a business rule as well as datastore rule.

                      So technically that check can be pushed the service layer before calling the dao method for an insert? (and removing the check inside the dao in this case)

                      Comment


                      • #41
                        Ok,

                        1.
                        With most databases insert (As well as other DML operations) via plain JDBC (it does not matter directly or by means of a stored procedure) would cause immediate exception in case of constraint violation. Only few databases (AFAIK) support deferred constraint and even then they are user relatively rarely (there are only a few compelling reasons to use them). So, if you have control over the database schema you practically always may ensure fail-fast behavior.

                        2.
                        Flush() concept is rather independent of data access implementation. It only assumes that is supports some kind of synchronization point at which all pending changes (if any) are propagated to the permanent storage and that this point may be called independently before commit. This assumption is rather sensible and so such method may be exposed by DAO layer. But even flush() does not protect against deferred constraints.

                        3. So I see only one generic solution - which both of us have already proposed - aspects with after-throwing advice that catches all exceptions thrown in the service layer and call exception resolver from dao layer (directly or via exception resolution service) to translate them into something more sensible.

                        But such solution requires rather a lot of efforts and in most cases I use flush in a service layer.

                        Regards,
                        Oleksandr

                        Originally posted by constv View Post
                        Yeah, it was not really relevant because it would still assume that the commit would have already occurred by the time the second thread attempts to insert, unless we put some crafty stuff inside the procedure, which would make it worse.

                        Interesting... It's not easy to come up with an elegant architecturally sound solution for this, it seems. The only way to actually capture such racing conflict is upon the end of the service transaction - on the service method. However, that means we may have to make an assumption about the data access/data source implementation - in the service. Unless, as you have suggested, we put some "finalizing" method on the DAO interface that would not really expose what it does but just attempt to catch such situation and throw an appropriate exception. Neither of the approaches sits well with me. I wonder what other people think.

                        I tend to think that in cases like that it may be reasonable to consider a sensible compromise. Not that I like this very much myself, but consider this... We do just a basic sanity check first - look up an existing user. If not found, proceed with an insert attempt. If a constraint violation exception occurs - specifically, perhaps, allow the user to retry once? (Ughhh...) The second time around, the sanity check should give a clear indication that the user already exists. What do you think? (In most cases, such stuff will not be needed, only in specific cases like ramoq's.) The ugly part about this, of course, is that the first time around the less clear message would still have to be shown to the user...

                        Comment


                        • #42
                          Ok, I guess this thing really got to me... Couldn't stop thinking about the solution that would be more or less acceptable. Let me know what you think guys...

                          Service:
                          Code:
                          public class UserServiceImpl implements UserService {
                              
                              private UserDao dao;          // reference to the data access object used by the service
                              
                          
                              /**
                               * Sets the reference to the DAO implementation for this service.
                               * @param theDao dao instance
                               */
                              @Required
                              public void setDao(UserDao theDao) {
                                  dao = theDao;
                              }
                              
                          
                              /**
                               * Creates a new user from the given User object and returns the updated object for the new user.
                               * @param theDao dao instance
                               * @return updated user object (perhaps, with the new unique ID generated, or/and additional status info, etc.)
                               */
                              @Transactional
                              public User createUser(User user) {
                                 // any business logic (if needed)
                                 try { 
                                      return dao.insertNewUser(user); // or, perform add'l logic and then return
                                 } catch (Throwable t) {
                                      String userEmailAddress = user.getEmailAddress();
                                      if (dao.lookupExistingUser(userEmailAddress) != null) {
                                           throw new DuplicateUserException("A user with email address " + userEmailAddress  + " already exists"); 
                                      } else {
                                           // wrapping here is actually redundant; you may just re-throw the original since the svc specific exception only states the obvious.
                                           throw new UserServiceException("Failed to create user for username/email address: " + userEmailAddress, t); 
                                      }
                                 }
                          
                              }
                               
                              ... other methods here
                          
                          }
                          As you see, we are catching any exception in the createUser service method without the necessity to know anything about the data tier implementation. If something goes wrong, since we are required to inform the user of an existing duplicate, if exists, we check whether the error was caused by a duplicate indeed. If yes, throw a specific Duplicate exception. Otherwise, just re-throw the original (or wrap it inside the Service specific exception, which I think is actually redundant.) No exception handling at all on the DAO side! No exposing the data tier detail to the service. Seems simple and answers the use case. Wonder what you guys think?

                          Thanks,
                          Constantine

                          Comment


                          • #43
                            Sorry, this solultion does not solve original problem - deferred exception.

                            It may be thrown not inside DAO method (unless it explicitly calls flush), but only on commit - i.e. after return from service method. So service method has no chances to catch it (unless explicitly flushes pending changes if any).

                            There is one more (smaller) problem tied to this solution - it is highly inadvisable to catch Throwable - and not rethrown immediately in a case of Error. Error's Javadoc states not without a reason

                            Code:
                            An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch.
                            Regards,
                            Oleksandr

                            Originally posted by constv View Post
                            Ok, I guess this thing really got to me... Couldn't stop thinking about the solution that would be more or less acceptable. Let me know what you think guys...

                            Service:
                            Code:
                            public class UserServiceImpl implements UserService {
                                
                                private UserDao dao;          // reference to the data access object used by the service
                                
                            
                                /**
                                 * Sets the reference to the DAO implementation for this service.
                                 * @param theDao dao instance
                                 */
                                @Required
                                public void setDao(UserDao theDao) {
                                    dao = theDao;
                                }
                                
                            
                                /**
                                 * Creates a new user from the given User object and returns the updated object for the new user.
                                 * @param theDao dao instance
                                 * @return updated user object (perhaps, with the new unique ID generated, or/and additional status info, etc.)
                                 */
                                @Transactional
                                public User createUser(User user) {
                                   // any business logic (if needed)
                                   try { 
                                        return dao.insertNewUser(user); // or, perform add'l logic and then return
                                   } catch (Throwable t) {
                                        String userEmailAddress = user.getEmailAddress();
                                        if (dao.lookupExistingUser(userEmailAddress) != null) {
                                             throw new DuplicateUserException("A user with email address " + userEmailAddress  + " already exists"); 
                                        } else {
                                             // wrapping here is actually redundant; you may just re-throw the original since the svc specific exception only states the obvious.
                                             throw new UserServiceException("Failed to create user for username/email address: " + userEmailAddress, t); 
                                        }
                                   }
                            
                                }
                                 
                                ... other methods here
                            
                            }
                            As you see, we are catching any exception in the createUser service method without the necessity to know anything about the data tier implementation. If something goes wrong, since we are required to inform the user of an existing duplicate, if exists, we check whether the error was caused by a duplicate indeed. If yes, throw a specific Duplicate exception. Otherwise, just re-throw the original (or wrap it inside the Service specific exception, which I think is actually redundant.) No exception handling at all on the DAO side! No exposing the data tier detail to the service. Seems simple and answers the use case. Wonder what you guys think?

                            Thanks,
                            Constantine

                            Comment


                            • #44
                              Sorry, this solultion does not solve original problem - deferred exception.

                              It may be thrown not inside DAO method (unless it explicitly calls flush), but only on commit - i.e. after return from service method. So service method has no chances to catch it (unless explicitly flushes pending changes if any).
                              Oops, you are absolutely correct, my bad. Any such wrapping should, of course, be around the transactional method itself. So, it should be done either via an aspect - if it is applicable to more than one case, or simply on the client around the call to the service. In either case, it answers the original question: very little explicit error handling (try/catches) should normally be done on the service and DAO methods. It is ultimately up to the client to take care of its own requirements, like in ramoq's case. Or, as you had suggested before in this thread - as a not preferred solution, the transaction could be extracted into a private method on the service, but, I agree, I would rather see the public API being explicitly transactional.

                              Comment


                              • #45
                                constv, thanks for the references on the ce and rte subject. On my last project I started to think about not using ce (suggested by one developer) and now I am convinced that I will not use it anymore. We had some bad experiences working on legacy projects with thousands of CE and it was a nightmare, the amount of lost information about the errors was insane.

                                Comment

                                Working...
                                X