8 Replies Latest reply: Jul 5, 2013 2:45 AM by BluShadow RSS

    Function is not returning correct value

    Nick4

      Hi

       

      I've created a function and I'm testing it in SQL Developer...

       

      CREATE OR REPLACE Function CheckUserHasRole(user_name IN varchar2, role_code IN varchar2)

        RETURN number

      IS

        rows_found number;

       

        CURSOR get_row IS

          SELECT COUNT(EMS_USER_ROLES.EMS_USER_ID) AS HAS_ROLE

          FROM EMS_ROLE

          INNER JOIN EMS_USER_ROLES ON EMS_ROLE.EMS_ROLE_ID = EMS_USER_ROLES.EMS_ROLE_ID

          INNER JOIN EMS_USER ON EMS_USER.EMS_USER_ID = EMS_USER_ROLES.EMS_USER_ID

          WHERE UPPER(EMS_USER.USERNAME) = UPPER(user_name) AND UPPER(EMS_ROLE.ROLE_CODE) = UPPER(role_code);

          

      BEGIN

        OPEN get_row;

        FETCH get_row INTO rows_found;

       

        IF get_row%notfound THEN

          rows_found := 0;

        END IF;

       

        CLOSE get_row;

       

        RETURN rows_found;

       

       

      EXCEPTION

        WHEN OTHERS THEN

          raise_application_error(-20001,'Authorisation Error - '||SQLCODE||' -ERROR- '||SQLERRM);

      END;

       

      Can anyone tell me why the following returns 1...

       

      select CheckUserHasRole('AUSER', 'PSR') as HASROLE FROM dual;

       

      but the running just the SQL part returns 0?...

       

         SELECT COUNT(EMS_USER_ROLES.EMS_USER_ID) AS HAS_ROLE

          FROM EMS_ROLE

          INNER JOIN EMS_USER_ROLES ON EMS_ROLE.EMS_ROLE_ID = EMS_USER_ROLES.EMS_ROLE_ID

          INNER JOIN EMS_USER ON EMS_USER.EMS_USER_ID = EMS_USER_ROLES.EMS_USER_ID

          WHERE UPPER(EMS_USER.USERNAME) = UPPER('AUSER') AND UPPER(EMS_ROLE.ROLE_CODE) = UPPER('PSR');

        • 1. Re: Function is not returning correct value
          JustinCave

          Don't use parameter names that are potentially column names.  That's causing your code to do something other than what you expect.  When you code

           

          WHERE UPPER(EMS_USER.USERNAME) = UPPER(user_name) 
            AND UPPER(EMS_ROLE.ROLE_CODE) = UPPER(role_code);
          

           

          I assume that your intention is to compare the ROLE_CODE column in the EMS_ROLE table against the function parameter ROLE_CODE.  The way name resolution works, however, is that Oracle first tries to resolve an identifier as a column in a table and only then looks for a local variable with that name.  Since there is a ROLE_CODE column in the EMS_TABLE, the second predicate compares the ROLE_CODE in EMS_ROLE against itself. 

           

          If you rename your parameters so that they do not conflict with your column names (P_USER_NAME and P_ROLE_CODE would be a common convention), the code should work.

           

          While the code should work, though, it's a bit convoluted.  There appears to be no need to declare a cursor or to do explicit cursor operations.  And there is no point to checking %NOTFOUND since the query is using an aggregate function that will always return a value.  Something like

           

          CREATE OR REPLACE Function CheckUserHasRole(p_user_name IN varchar2, p_role_code IN varchar2)
            RETURN number
          IS
            l_num_rows number;
          BEGIN
              SELECT COUNT(EMS_USER_ROLES.EMS_USER_ID) AS HAS_ROLE
               INTO l_num_rows
          
              FROM EMS_ROLE
              INNER JOIN EMS_USER_ROLES ON EMS_ROLE.EMS_ROLE_ID = EMS_USER_ROLES.EMS_ROLE_ID
              INNER JOIN EMS_USER ON EMS_USER.EMS_USER_ID = EMS_USER_ROLES.EMS_USER_ID
              WHERE UPPER(EMS_USER.USERNAME) = UPPER(p_user_name) 
                AND UPPER(EMS_ROLE.ROLE_CODE) = UPPER(p_role_code);
          
          
            RETURN l_num_rows;
          EXCEPTION
            WHEN OTHERS THEN
              raise_application_error(-20001,'Authorisation Error - '||SQLCODE||' -ERROR- '||SQLERRM);
          END;
          

           

          would seem simpler (assuming it really makes sense to have a WHEN OTHERS exception handler here rather than just letting the exception propagate).

           

          Justin

          • 2. Re: Function is not returning correct value
            Nick4

            Thanks Justin, that fixed it!

             

            Your function worked, then I renamed my parameters and it also worked...

            • 3. Re: Function is not returning correct value
              dariyoosh

              Hi,

               

              In addition to what Paul said, I don't really see why you need to use a cursor inside your function. An embedded static SQL does the job. Something like this:

               

              CREATE OR REPLACE Function CheckUserHasRole
              (
                  user_name IN varchar2,
                  role_code IN varchar2
              )
              RETURN number
              IS
              BEGIN
                  <<bk>>
                  DECLARE
                      rowCnt NUMBER;
                  BEGIN
                      SELECT  count(t2.ems_user_id) has_role
                      INTO    bk.rowCnt
                      FROM    ems_role t1
                          INNER JOIN ems_user_roles t2
                              ON (t1.ems_role_id = t2.ems_role_id)
                          INNER JOIN ems_user t3
                              ON t3.ems_user_id = t2.ems_user_id
                      WHERE   upper(t3.username) = upper(user_name) AND
                              upper(t1.role_code) = upper(role_code);
                             
                      RETURN rowCnt;
                  END;
              END CheckUserHasRole;

               

              Regards,

              Dariyoosh

              • 4. Re: Function is not returning correct value
                BluShadow
                JustinCave wrote:
                Don't use parameter names that are potentially column names.  That's causing your code to do something other than what you expect.  When you code
                WHERE UPPER(EMS_USER.USERNAME) = UPPER(user_name) 
                  AND UPPER(EMS_ROLE.ROLE_CODE) = UPPER(role_code);
                I'd disagree slightly with that.
                I'd say, don't use parameter names that are potentially column names, if you're not aware what you're doing (slight catch 22 though hehe!)
                Better to fully qualify the names:
                WHERE UPPER(EMS_USER.USERNAME) = UPPER(checkuserhasrole.user_name)
                AND UPPER(EMS_ROLE.ROLE_CODE) = UPPER(checkuserhasrole.role_code)
                and then it's clear what scope the variables/parameters are being picked up from, whilst preventing ambiguity in the SQL statement itself.
                • 5. Re: Function is not returning correct value
                  Nick4

                  That sounds like a good idea... don't need to worry about whether the names are the same at all.

                   

                  Thanks

                  • 6. Re: Function is not returning correct value
                    Nick4

                    Yes, Justin mentioned the use of a cursor being unnecessary. I based my function on an example I found on another website which did declare the cursor.

                     

                    Thanks

                    • 7. Re: Function is not returning correct value
                      rp0428

                      That sounds like a good idea... don't need to worry about whether the names are the same at all.

                      No - that is a TERRIBLE idea. Best practices are to name your parameters such that it is abundantly clear that the name refers to a parameter.

                       

                      You were well advised by Justin about the naming and I strongly suggest you follow that advice.

                      • 8. Re: Function is not returning correct value
                        BluShadow

                        rp0428 wrote:

                         

                        That sounds like a good idea... don't need to worry about whether the names are the same at all.

                        No - that is a TERRIBLE idea. Best practices are to name your parameters such that it is abundantly clear that the name refers to a parameter.

                         

                        You were well advised by Justin about the naming and I strongly suggest you follow that advice.

                         

                        Terrible? Really?

                         

                        Does it matter if it's a parameter, or a local variable etc?  Whilst naming parameters and local variables with prefixes of "p_" or "v_" etc. is one way of doing it that's acceptable (though somewhat of an "old" style of programming - but yes I do still do this myself sometimes), it's also perfectly acceptable to prefix the variable with the scope name (i.e. the procedure or function or package name etc.)  it's quite clear where it's coming from then regardless of what type of variable it is.  Not sure what makes you consider it "terrible".