Code cleanup: post-authentication in Application Express

Recently ran across this procedure, defined in the Post Authentication field in an Appliation Expression application. I encourage you to look it over and come up with your list of things you'd want to change about it. I leave a whole lot of white space below the code block so you can view the end before seeing what I have to say about it.

1:  procedure post_auth_7048783549465082709 is  
2:  begin  
3:  APEX_UTIL.RESET_AUTHORIZATIONS;  
4:    
5:  for c1 in (  
6:    select count(*) cnt  
7:     from app_users  
8:    where username = :APP_USER  
9:     and role_name = 'ADMINISTRATOR' )  
10:  loop  
11:    if c1.cnt > 0 then  
12:     :IS_ADMIN := 'TRUE';  
13:    else  
14:     :IS_ADMIN := 'FALSE';  
15:    end if;  
16:  end loop;  
17:    
18:  for c1 in (  
19:    select count(*) cnt  
20:     from app_users  
21:    where username = :APP_USER  
22:     and role_name = 'CONTRIBUTOR' )  
23:  loop  
24:    if c1.cnt > 0 then  
25:     :IS_CONTRIBUTOR := 'TRUE';  
26:    else  
27:     :IS_CONTRIBUTOR := 'FALSE';  
28:    end if;  
29:  end loop;  
30:    
31:  for c1 in (  
32:    select count(*) cnt  
33:     from app_users  
34:    where username = :APP_USER  
35:     and role_name = 'TESTER' )  
36:  loop  
37:    if c1.cnt > 0 then  
38:     :IS_TESTER := 'TRUE';  
39:    else  
40:     :IS_TESTER := 'FALSE';  
41:    end if;  
42:  end loop;  
43:    
44:  for c1 in (  
45:    select count(*) cnt  
46:     from app_users  
47:    where username = :APP_USER  
48:     and role_name = 'REPORTING' )  
49:  loop  
50:    if c1.cnt > 0 then  
51:     :IS_REPORTING := 'TRUE';  
52:    else  
53:     :IS_REPORTING := 'FALSE';  
54:    end if;  
55:  end loop;  
56:    
57:  for c1 in (  
58:    select substr(lower(:APP_USER), instr(:APP_USER, '@')+1) email  
59:     from dual  
60:  )  
61:  loop  
62:    if c1.email = 'our_domain.com' then  
63:     :is_employee := 'TRUE';  
64:    else  
65:     :is_employee := 'FALSE';  
66:    end if;  
67:  end loop;  
68:    
69:  for c1 in (  
70:    select count(*) cnt  
71:     from app_users  
72:    where username = :APP_USER  
73:     and role_name = 'SUPER ADMINISTRATOR' )  
74:  loop  
75:    if c1.cnt > 0 then  
76:     :IS_SUPERADMIN := 'TRUE';  
77:    else  
78:     :IS_SUPERADMIN := 'FALSE';  
79:    end if;  
80:  end loop;  
81:    
82:  end;  


Don't look down below yet.

What do you think is wrong?

The more capable you are at analyzing this (and your own) code all by your lonesome, the more valuable you will be to your dev teammates and employer.












Right then. Let's take a look at how we might clean this up. Here are my concerns:
  1. Poor formatting: Even if you don't like my style of upper-casing all keywords, the lack of indentation is a significant impediment to reading code.
  2. Use of cursor FOR loop with a single row query (SELECT COUNT(*)): Cursor FOR loops are for fetching multiple rows of data. If you know for sure that there is just one row, using a cursor FOR loop just means you are being lazy, and not in a good way (you don't have to declare a record or variable to accept the fetched value).
  3. Use of COUNT(*) to determine if there is at least one row: COUNT(*) answers the question "How many have I got?" when the question being asked here is "Is there at least one row?".  What if there are 10,000,000 rows? The logic works, but you could spend a few extra CPU cycles getting to the answer. And, as with the use of the cursor FOR loop, it's misleading and confusing.
  4. Repetitive code: If you need the same functionality again and again, don't copy and paste. Ugh. Instead, apply your relational database normalization skills to your code: create a nested subprogram and call that multiple times.
  5. SELECT FROM dual: No, please, no! If you are running 11.1 or higher, there is almost no reason left to use a SELECT FROM dual instead of a native PL/SQL assignment. Certainly this is not necessary for an expression involving SUBSTR.
  6. Too much code inside Application Express: whenever possible move your PL/SQL code to packages compiled into the database. It is easier to manage from there.
Did I miss anything? Let me know!

Here's my first pass on a cleanup of this procedure:

PROCEDURE post_auth_7048783549465082709
IS
   PROCEDURE set_value (role_in IN VARCHAR2, item_out OUT VARCHAR2)
   IS
      FUNCTION role_exists (role_in IN VARCHAR2)
         RETURN BOOLEAN
      IS
      BEGIN
         SELECT 'x'
           INTO l_dummy
           FROM app_users
          WHERE username = :app_user AND role_name = role_in;

         RETURN TRUE;
      EXCEPTION
         WHEN NO_DATA_FOUND
         THEN
            RETURN FALSE;
      END;
   BEGIN
      item_out := CASE WHEN role_exists (role_in) THEN 'TRUE' ELSE 'FALSE' END;
   END;
BEGIN
   APEX_UTIL.RESET_AUTHORIZATIONS;
   
   set_value ('ADMINISTRATOR', :is_admin);
   set_value ('CONTRIBUTOR', :is_contributor);
   set_value ('TESTER', :is_tester);
   set_value ('REPORTING', :is_reporting);
   set_value ('SUPER ADMINISTRATOR', :is_superadmin);

   :is_employee :=
      CASE
         WHEN SUBSTR (LOWER ( :app_user), INSTR ( :app_user, '@') + 1) =
                 'our_domain.com'
         THEN
            'TRUE'
         ELSE
            'FALSE'
      END;
END;

This rewrite covers the first 5 concerns I listed. Gone are the cursor FOR loops, the COUNT(*), the repetitive code, the SELECT FROM dual. I shift from IF statements to CASE expressions, allowing for more concise and cleaner code.

Now let's take a look at how we might tackle concern #6 ("Whenever possible move your PL/SQL code to packages compiled into the database."). The app_post_authenticate procedure is a database object. It has pretty much the same code as my first rewrite, but instead of referencing host variables directly inside the procedure, I move them into the parameter list.

CREATE OR REPLACE PROCEDURE app_post_authenticate (
   app_user_in       IN     VARCHAR2,
   admin_out            OUT VARCHAR2,
   contributor_out      OUT VARCHAR2,
   tester_out           OUT VARCHAR2,
   reporting_out        OUT VARCHAR2,
   super_admin_out      OUT VARCHAR2,
   is_employee_out      OUT VARCHAR2)
IS
   PROCEDURE set_value (role_in IN VARCHAR2, item_out OUT VARCHAR2)
   IS
      FUNCTION role_exists (role_in IN VARCHAR2)
         RETURN BOOLEAN
      IS
      BEGIN
         SELECT 'x'
           INTO l_dummy
           FROM app_users
          WHERE username = app_user_in AND role_name = role_in;

         RETURN TRUE;
      EXCEPTION
         WHEN NO_DATA_FOUND
         THEN
            RETURN FALSE;
      END;
   BEGIN
      item_out := CASE WHEN role_exists (role_in) THEN 'TRUE' ELSE 'FALSE' END;
   END;
BEGIN
   APEX_UTIL.RESET_AUTHORIZATIONS;

   set_value ('ADMINISTRATOR', admin_out);
   set_value ('CONTRIBUTOR', contributor_out);
   set_value ('TESTER', tester_out);
   set_value ('REPORTING', reporting_out);
   set_value ('SUPER ADMINISTRATOR', superadmin_out);

   is_employee_out :=
      CASE
         WHEN SUBSTR (LOWER (app_user_in), INSTR (app_user_in, '@') + 1) =
                 'our_domain.com'
         THEN
            'TRUE'
         ELSE
            'FALSE'
      END;
END;

And then in Application Express, I have only this left:

PROCEDURE post_auth_7048783549465082709
IS
BEGIN
   app_post_authenticate ( :app_user,
                          :is_admin,
                          :is_contributor,
                          :is_tester,
                          :is_reporting,
                          :is_superadmin,
                          :is_employee);
END;

Sure, you could argue that by moving all the code to the database, you no longer can see what is going on in your application.

I would counter with: how often do you need to see this logic? Probably rarely. And most of the logic involved is already in the database: the app_users table, the literal values needed to "match up" with those rows in the table, etc. In addition, besides setting values of application items, there is no UI-specific logic in the procedure. That's another reason to move the code out of the application.

Comments

  1. Steven, to optimise furher : Instead of calling a function for each role I would set all to FALSE and use a cursor for all roles for app user and in the loop use "case" to set TRUE for roles returned. This way you have only one select statement executed instead of one per role.

    ReplyDelete
  2. I like that, Jean-Marc. Here's the only problem I see: we have a hard-coded list of application items. What happens if there is a role that does not match up to one of those? And how would associate the value fetched from the cursor FOR loop to the item? Care to post some code? :-)

    ReplyDelete
  3. Hi, Steven

    Very good points you make in this post. But like Jean-Marc I'll also add that when you have code that have to repeatedly call the same SQL, I'll try to look for ways to avoid repeated SQL calls and replace it with single SQL calls.


    I'll have to assume that (username, role_name) is unique in the table? Otherwise your revised code would have to worry about TOO_MANY_ROWS or add a AND ROWNUM <= 1.

    So given that assumption, a couple ways to have only one SELECT statement.

    First like Jean-Marc describes:


    CREATE OR REPLACE PROCEDURE app_post_authenticate (
    app_user_in IN VARCHAR2,
    admin_out OUT VARCHAR2,
    contributor_out OUT VARCHAR2,
    tester_out OUT VARCHAR2,
    reporting_out OUT VARCHAR2,
    super_admin_out OUT VARCHAR2,
    is_employee_out OUT VARCHAR2)
    IS
    BEGIN
    APEX_UTIL.RESET_AUTHORIZATIONS;

    admin_out := 'FALSE';
    contributor_out := 'FALSE';
    tester_out := 'FALSE';
    reporting_out := 'FALSE';
    superadmin_out := 'FALSE';

    for role in (
    select role_name
    from app_users
    where username = app_user_in
    and role_name in (
    'ADMINISTRATOR'
    , 'CONTRIBUTOR'
    , 'TESTER'
    , 'REPORTING'
    , 'SUPER ADMINISTRATOR'
    )
    ) loop
    case role.role_name
    when 'ADMINISTRATOR' then
    admin_out := 'TRUE';
    when 'CONTRIBUTOR' then
    contributor_out := 'TRUE';
    when 'TESTER' then
    tester_out := 'TRUE';
    when 'REPORTING' then
    reporting_out := 'TRUE';
    when 'SUPER ADMINISTRATOR' then
    superadmin_out := 'TRUE';
    end case;
    end loop;

    is_employee_out :=
    CASE
    WHEN SUBSTR (LOWER (app_user_in), INSTR (app_user_in, '@') + 1) =
    'our_domain.com'
    THEN
    'TRUE'
    ELSE
    'FALSE'
    END;
    END;


    Nice and simple and readable :-)

    (Alternative comes in a seperate reply due to 4096 char limit.)


    /Kim

    ReplyDelete
  4. (Part 2:)

    An alternative eliminates the need for the CASE logic in PL/SQL and moves all logic into SQL. It has the advantage of only fetching a single row of 5 columns instead of 5 rows of 1 column, but at the cost of a more complex and less readable SQL:


    CREATE OR REPLACE PROCEDURE app_post_authenticate (
    app_user_in IN VARCHAR2,
    admin_out OUT VARCHAR2,
    contributor_out OUT VARCHAR2,
    tester_out OUT VARCHAR2,
    reporting_out OUT VARCHAR2,
    super_admin_out OUT VARCHAR2,
    is_employee_out OUT VARCHAR2)
    IS
    BEGIN
    APEX_UTIL.RESET_AUTHORIZATIONS;

    begin
    select case admin_exist when 0 then 'FALSE' else 'TRUE' end
    , case contributor_exist when 0 then 'FALSE' else 'TRUE' end
    , case tester_exist when 0 then 'FALSE' else 'TRUE' end
    , case reporting_exist when 0 then 'FALSE' else 'TRUE' end
    , case superadmin_exist when 0 then 'FALSE' else 'TRUE' end
    into admin_out
    , contributor_out
    , tester_out
    , reporting_out
    , superadmin_out
    from (
    select role_name
    from app_users
    where username = app_user_in
    and role_name in (
    'ADMINISTRATOR'
    , 'CONTRIBUTOR'
    , 'TESTER'
    , 'REPORTING'
    , 'SUPER ADMINISTRATOR'
    )
    )
    pivot (
    count(*) as exist
    for role_name in (
    'ADMINISTRATOR' as admin
    , 'CONTRIBUTOR' as contributor
    , 'TESTER' as tester
    , 'REPORTING' as reporting
    , 'SUPER ADMINISTRATOR' as superadmin
    )
    )
    ;
    exception
    when no_data_found then
    admin_out := 'FALSE';
    contributor_out := 'FALSE';
    tester_out := 'FALSE';
    reporting_out := 'FALSE';
    superadmin_out := 'FALSE';
    end;

    is_employee_out :=
    CASE
    WHEN SUBSTR (LOWER (app_user_in), INSTR (app_user_in, '@') + 1) =
    'our_domain.com'
    THEN
    'TRUE'
    ELSE
    'FALSE'
    END;
    END;


    (An aggregate function is needed in order to PIVOT, hence the count(*) - but as we know we have a unique role_name here, it hardly matters ;-)

    /Kim

    ReplyDelete
  5. Steven, here is some code + an add. comment :
    If the APEX application is to be used frequently & that db is 11+, I will maybe choose to keep your function with app_user in its parameters and cache-enabled the function.
    But if we start using caching (result cache, smart flash cache or even in-memory to drastically speed things) for all little things we will completely stop taking care of our code, don't we ? ;)

    Code:

    PROCEDURE post_auth_7048783549465082709
    IS
    BEGIN

    :is_admin:='FALSE';
    :is_contributor:='FALSE';
    :is_tester:='FALSE';
    :is_reporting:='FALSE';
    :is_superadmin:='FALSE';

    for c1 in (SELECT role_name
    FROM app_users
    WHERE username = :app_user)
    loop
    CASE
    WHEN c1.role_name='ADMINISTRATOR' THEN :is_admin:='TRUE';
    WHEN c1.role_name='CONTRIBUTOR' THEN :is_contributor:='TRUE';
    WHEN c1.role_name='TESTER' THEN :is_tester:='TRUE';
    WHEN c1.role_name='REPORTING' THEN :is_reporting:='TRUE';
    ELSE c1.role_name='SUPER ADMINISTRATOR' THEN :is_superadmin:='TRUE';
    END;
    end loop;

    is_employee :=
    CASE
    WHEN SUBSTR (LOWER ( :app_user), INSTR ( :app_user, '@') + 1) =
    'our_domain.com'
    THEN
    'TRUE'
    ELSE
    'FALSE'
    END;
    END;

    ReplyDelete
  6. So, Kim and others: do you feel that the additional complexity of your second solution - to remove the CASE from PL/SQL is justified? Seems like at a minimum one would want to turn such solutions into a "learning moment" by adding comments so that anyone working on it later would (a) know what the heck is going on and (b) be inspired to use it elsewhere.

    ReplyDelete
  7. Actually, no, not in this case.

    In this case the extremely minor benefit of 1 row with 5 columns instead of 5 rows of 1 column does *not* justify the complexity of pivoting the SQL. That one I just wrote as learning example to give a method that might be useful for larger cases :-)

    It even just might (haven't tested it, though) make the SQL engine work more than the work saved for the PL/SQL engine - in this simple case. There can easily be more complex cases where it makes more sense to move logic from PL/SQL to SQL, but this one isn't one of them.

    But the solution with one FOR loop and a CASE statement I believe is fully justified as it turns 5 SQL calls into 1 - even though it does not use a self-documenting local procedure ;-) It's quite understandable as is.

    I'm a SQL guy so I'll always at least *think* about how I could possibly do *anything* in a single SQL statement. But yes, I can easily fall into the trap of adding unnecessary complexity in order to shave milliseconds of a mixed SQL + PL/SQL solution. Plus the complex SQL has a higher risk of plan *in*stability due to being more difficult to optimize.

    It's always a tradeoff and it's a matter of finding the best balance between SQL and PL/SQL :-)

    ReplyDelete
  8. Thanks, Kim. Very thoughtful and balanced response. And, for the record, I am perfectly fine with not having even a single nested subprogram if it's already quite understandable (and lots of the logic has been pushed into SQL).

    ReplyDelete
  9. Steven

    When we discussed this at the Collaborate CodeTalk session someone mentioned using bulk collect instead of the for loop.

    Thinking about this a bit more, I realised it would be cool if bulk collect allowed you to define your own indicies for the target collection. In this case you could index by varchar2, with the role names as the indicies. You could then assign the variables by referencing the role name.

    The code for this would look something like this:

    select case when au.role_name is null then 'FALSE' else 'TRUE' end
    bulk collect into user_roles
    indicies rs.r
    from (select 'ADMINISTRATOR' r from dual union all
    select 'CONTRIBUTOR' r from dual union all
    select 'TESTER' r from dual union all
    select 'REPORTING' r from dual union all
    select 'SUPER ADMINISTRATOR' r from dual) roles rs
    left join app_users au
    on rs.r = au.role_name;

    admin_out := user_roles('ADMINISTRATOR');
    contributor_out := user_roles('CONTRIBUTOR');
    tester_out := user_roles('TESTER');
    reporting_out := user_roles('REPORTING');
    superadmin_out := user_roles('SUPER ADMINISTRATOR');

    What do you think? Does being able to define your own indicies for bulk collect have value?

    ReplyDelete
  10. Great idea, Chris - and not only because I thought of it first. :-) I like how it offers a neat implementation in this example. I was thinking (and did bring this up with Bryn Llewellyn as an enhancement) of adding an INDEX BY clause similar to the TYPE statement for creating a collation type as in:

    SELECT
    BULK COLLECT INTO
    INDEX BY

    I believe this is an elegant extension from the current syntax and would simplify any number of scenarios for PL/SQL programmers.

    Having said that, I cannot speak to when or if this might actually appear in the PL/SQL language.

    ReplyDelete

Post a Comment

Popular posts from this blog

Table Functions, Part 1: Introduction and Exploration

Recommendations for unit testing PL/SQL programs

The future of Oracle PL/SQL: some thoughts on Sten Vesterli's thoughts