Forum OpenACS Development: content_folder.rename

Collapse
Posted by Samer Abukhait on
I found this code under package content_folder
procedure rename (
  folder_id	 in cr_folders.folder_id%TYPE,
  name	         in cr_items.name%TYPE default null,
  label	         in cr_folders.label%TYPE default null,
  description    in cr_folders.description%TYPE default null
) is
begin

  if name is not null then content_item.rename(folder_id, name); end if;

  if label is not null and description is not null then

    update cr_folders
      set label = label,
      description = description
      where folder_id = folder_id;

  elsif label is not null and description is null then

    update cr_folders
      set label = label
      where folder_id = folder_id;

  end if;

end rename;

    2 things:
  • rename and delete cannot be used anymore in Oracle9+, all PL/SQL names should be converted.
  • The update statements do nothing; variables should be prefixed with the function name.

    I fixed it this way:

    procedure re_name (
      folder_id	 in cr_folders.folder_id%TYPE,
      name	         in cr_items.name%TYPE default null,
      label	         in cr_folders.label%TYPE default null,
      description    in cr_folders.description%TYPE default null
    ) is
    begin
    
      if name is not null then content_item.re_name(folder_id, name); end if;
    
      if label is not null and description is not null then
          update cr_folders
          set label = re_name.label,
          description = re_name.description
          where folder_id = re_name.folder_id;
      elsif label is not null and description is null then
          update cr_folders
          set label = re_name.label
          where folder_id = re_name.folder_id;
      end if;
    
    end re_name;
    
    
Collapse
2: Re: content_folder.rename (response to 1)
Posted by Jeff Davis on
please book it as a bug, also I think the convention we ended
up with was .del and .ren for these functions.

Thanks for finding it...

Collapse
3: Re: content_folder.rename (response to 1)
Posted by Samer Abukhait on
fine,

I got sure that I have no more rename and delete using:

select * from user_source where text like '% delete (%' or text like '% rename (%'

But how about the other problem? Calling matching variables in a query without suffixing them??
It is hard to be searched

Collapse
4: Re: content_folder.rename (response to 1)
Posted by Samer Abukhait on
reported as bug #1193
Collapse
Posted by Dirk Gomez on

Samer, thanks for pointing this out. An early aD style guide recommended the use of <proc_name>.variable_name.

I think we should just drop this coding style recommendation and move towards to variables with prefixes e g. v_ or m_. I agree that the aD style guide looks nicer and cleaner, but it is error-prone and it is next to impossible to track down these errors except with constant code review. And why not just avoid bugs that are hard to be tracked down - if it is so easily done?

Collapse
7: Re: content_folder.rename (response to 4)
Posted by Dirk Gomez on
The problem is that PL/SQL needs to tell apart variables from column names. If the variable name happens to also be a column name, you must prefix it with the proc name, otherwise you will have a clause that is short circuited.
Collapse
9: Re: content_folder.rename (response to 4)
Posted by Dirk Gomez on

Here's a good example where I think that this naming convention under Oracle is dangerous:


        procedure del (
                calendar_id             in calendars.calendar_id%TYPE
        )
        is
  
        begin
                  -- First erase all the item relate to this calendar.
                delete from     calendars 
                where           calendar_id = calendar.del.calendar_id;
 
                  -- Delete all privileges associate with this calendar
                delete from     acs_permissions 
                where           object_id = calendar.del.calendar_id;

                  -- Delete all privilges of the cal_items that's associated 
                  -- with this calendar
                delete from     acs_permissions
                where           object_id in (
                                        select  cal_item_id
                                        from    cal_items
                                        where   on_which_calendar = calendar.del.calendar_id                                                                                                                                                         
                                );
                        
 
                acs_object.del(calendar_id);
        end del;

It admittedly is more stylish, but it is error-prone (without throwing errors) and is potentially dangerous because it may delete loads of items.

Let me dig up a coding style proposal from Feuerstein's books once I am back from the Hamburg bug bash.

Collapse
6: Re: content_folder.rename (response to 1)
Posted by Tom Jackson on

Is proc_name.variable_name a coding style? I thought it was namespace qualification? In Oracle sometimes you need it. Obviously it doesn't exist in pg.

Collapse
8: Re: content_folder.rename (response to 1)
Posted by Jade Rubick on
We do have naming conventions for some things:

https://openacs.org/faq/one-faq?faq_id=43841#43867

Let me know if we should add to that.

I agree with Dirk, emphatically.

In PL/SQL, referencing variables in the proc_name.variable_name style is overly verbose, hard to read, error prone, and basically disgusting in general. I was never aware of any aD "style guide" saying to do it that way, but if there was, that style guide was very misguided. IMNSHO.

If there was a convenient concise namespace notation that meant the same thing and did not constantly repeat the name of the function or procedure everywhere (which you may well want to change...), something like, this.variable_name, say, then that would be nice and maybe worth using. But there isn't, and constantly referring to the my_big_long_proc_name_foo.object_id or whatever variable is just plain obnoxious. Using p_update_date or q_update_date or a similar convention like that is much better. And the fact that this same convention also work in PostgreSQL is a nice added bonus.

Oops note that in my little example above, I meant to say, "Using p_object_id or v_object_id or some similar convention is much better."
It is still here: https://openacs.org/doc/openacs-4-6-3/eng-standards-plsql.html - Point 6.

Maybe we should just use the PostgreSQL conventions in Oracle.

Guess that sounds like a TIP right?

Maybe there is a way to do such a conversion semi-automatically, have to think about it.

Andrew, I'm guessing you mean p_* for the passed in parameters, and v_* for the local variables. This convention makes it easy to figure out where a variable came from without having to trace through a bunch of code just to make sure. However, once you have done these two simple changes, sometimes you still need the package.proc.var name qualifiers in Oracle. I seem to remember that cursor packages are picky if you don't do everything exactly right.

Tom, I didn't remember exactly conventions OpenACS has used before, but yes what you describe for p_* and v_* variables sounds excellent to me.

(At aD, I think the rest of the Muniversal team and I had written reams of custom PL/SQL in a similar style before most of the aD "core team" started using PL/SQL much at all. [grumble grumble])

If you occasionally need to use Oracle's namespace qualification feature on local variables, fine, that's what it's there for. But it would would much nicer to see it only where it's actually needed, not willy nilly all over the place in order to make the variable names as hard to read as possible.

That old style guide Dirk links to above seems awfully mis-guided on this relatively trivial point. Note that the authors seemed to confuse two unrelated things, they say: "Name parameters as simply as possible" and "To achieve this we must fully qualify arguements passed into procedures" - nonsense, the second statement does not follow from the first.

Personally it never bothered me at all to call PL/SQL functions with parameters named "p_foo" rather than "foo", but if you really hate that, that does not mean that your only other option is to use namespace qualification all over the place. Rather obviously, you can simply declare a "p_foo" or "v_foo" variables near the top of your function, and initialize "v_foo" to the passed in value of "foo". Not necessarily ideal, but for a long function, that is going to be much clearer than prefixing the variable with the function name dozens of times throughout the code.

Andrew, I agree, using the function name to qualify a local variable is a useless idea, an idea that is pretty much solved by appending p_ to every passed in parameter, or in pg, aliasing the same way. And every locally declared variable gets appended with v_. The place to be verbose, if at all, in a variable name is in the meat of the name, not in some prefix. Of course what is clear to me might be confusing to someone else. So the debate may continue.

Collapse
16: Re: content_folder.rename (response to 1)
Posted by Don Baccus on
If you look at the link provided above you'll see that we did settle on the p_* and v_* notation. I guess that never got migrated to our docs.

The problem we face (as always) is legacy code, there are lots of queries out there using named parameters ("param_name => value") in calls to PL/SQL and breaking them is something we were loath to do, given that we knew some folks would be migrated aD 4.2 sites to OpenACS 4 etc.

For new stuff though I'd love to see us settle on that convention if others agree. Do we have something like consensus among those posting to this thread? If so, let's TIP it.

Collapse
Posted by Dirk Gomez on
I am sure that we will find a way to get rid of as many bad examples as possible. Because bad code breeds bad code we should always work eagerly towards this goal :)

It should be possible to "script"-ize at least parts of such a conversion. I will dig up the sed book.

Otherwise I agree: let's get rid of a prefixed variables at least for all new packages.

Collapse
Posted by Tom Jackson on

Yes, I agree. I thought that was the convention. Everytime I have to look at old code I get confused by the var names. There isn't much to do about old code, but new stuff probably would be much easier to understand and maintain by following these two simple conventions.