Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add default values for all attributes from cursor #3

Open
jgebal opened this issue May 27, 2017 · 21 comments
Open

Add default values for all attributes from cursor #3

jgebal opened this issue May 27, 2017 · 21 comments

Comments

@jgebal
Copy link
Contributor

jgebal commented May 27, 2017

I see great and ability to use this API as part of utPLSQL v3 to generate setup/cleanup API's for unit tests.
The only thing that is missing is ability to pass default values for all attributes for insert API.

I'm looking for an API that would insert a row and return inserted data.
The insert row API should require 0 parameters (insert a default dummy row into specific table).
It should also expose all attributes of row to be set if needed.
Example of generated API signature I'm thinking of:

function insert_emp(
id integer := null -- PK -> (could be also emp_id_seq.nextval or -1)
first_name varchar2 := 'Chuck',
last_name varchar2 := 'Norris'
) return emp%rowtye;

So then in my Unit Test I can call:

  ...insert_emp(first_name=>'Daddy', last_name=>'Cool');

--and test my PLSQL code 
  ut.expect( count_emp_by_name('Dady Cool').count ).to_equal(1);

I was thinking that your generator API could be extended to accept additional parameter p_default_values_crsr sys_refcursor so that it could be used like this:

DECLARE
  l_crsr SYS_REFCURSR;
BEGIN
  FOR i IN (SELECT table_name FROM user_tables /*WHERE...*/) LOOP
    open l_crsr for `SELECT * FROM '||i.table_name;
    your_install_schema.om_tapigen.compile_api(
      p_table_name                 => i.table_name,
      p_reuse_existing_api_params  => FALSE,
      p_col_prefix_in_method_names => TRUE,
	    p_enable_insertion_of_rows   => TRUE,
      p_default_values_for_insert  => l_crsr,
	    p_enable_update_of_rows      => TRUE,
      p_enable_deletion_of_rows    => FALSE,
      p_enable_generic_change_log  => FALSE,
	    p_enable_dml_view            => FALSE,
      p_sequence_name              => NULL);  
  END LOOP;
END;

Do you think it would be something fitting into your API project?

@aborngra
Copy link
Member

Hi jgebal,

we already thought about default values for the API. As you suggested, it would be very usefull for CREATE operations. For other operations like UPDATE or CREATE_OR_UPDATE would be "risky" to use default values. Instead of a cursor, we have the idea to take the default value from the table column directly, I think it's the simplest way ;-)
So in our next version, default values will be added for the CREATE operations.

Best regards
André

@ogobrecht
Copy link
Member

Hi Jacek,

I like your idea and as André mentioned we already thought about it - but not so deep. We wanted only to provide the table defaults - but additional custom defaults would be also great.

I was thinking a little bit about your idea and I have the following comments/ideas:

To be backward compatible I don't want to change the default create_row methods (I use it already in some projects and breaking backward compatibility would be generate much work - in my projects and maybe also in some unknown projects of other users).

How about two new functions per generated API? (@aborngra what do you think?)

  • create_row_table_defaults (with the defaults defined for the table, thats what we thought about defaults)
  • create_row_custom_defaults (the one you want to have and I also :-)

There is one problem: We save the API creation parameters in the API spec to be able to recreate the API without any given parameter, that is also the reason to have the parameter c_reuse_existing_api_params with the default value true. We use XML coded parameters in the spec to be able to read the parameters even the package is invalid because of table changes or other reasons. So the question is: What do you think - should the parameter for the custom defaults a ref cursor and we convert it to XML for the spec or it is better that the user defines already a XML for the default values and we provide may be a convenient function to convert a ref cursor into XML?

One comment about our method names: We were inspired by the CRUD acronym for Create, Read, Update and Delete - that is the reason to use create_row instead of insert_xxx or ins_xxx.

And yes, there is room for improvements - thanks for all your suggestions and ideas :-)

Best regards
Ottmar

@jgebal
Copy link
Contributor Author

jgebal commented May 28, 2017

Hi Guys,
Thanks for all the hard work you've put into the project.
I love the CRUD API approach and the way you parametrised the generator.

Adding default values should not be breaking change I think as, currently whenever you call the insert API, you need to specify all attributes.
The only way it would be considered a breaking change is when you modify table. The code using API with defaults would still work (After regeneration) while the code using API without defaults would not.

Maybe the defautls/no defaults should be parameter of generation?
It seems like two different use-cases.
In general more parameters give more control, just the default behavior should be well defined :)

We're thinking of a very specific use-case for table API's (as helpers for unit tests).
I might do a demo project at some pint that would show how it is beneficial for unit testing.

Thanks
Jacek

@ogobrecht
Copy link
Member

ogobrecht commented May 28, 2017

Hi Jacek,

are you constantly working? ;-)

You are right: The defaults are NOT invalidating the API on table changes - and this is a nice thing - to be remembered to recreate the APIs when table changes occur. For that I (hopefully also André) don't want to change the existing create_row logic - not to have default values was a design decision at our project start.

I like the idea to have two additional create_row methods like mentioned before. With this it is also clear, which method does what regarding the defaults. And yes, we need parameters for that - thats the reason for my question before: which should be the parameter type for the creation of the create_row_custom_defaults - a ref cursor or xml? What do you think is better to use (please reread my previous comment)

Suggestion:

  • function: create_row_table_defaults (parameter: p_enable_table_defaults boolean default false)
  • function: create_row_custom_defaults (parameter: p_custom_table_defaults xmltype or sys_refcursor? default null, we need to save the defaults in the package spec for reuse/recreating)

Best regards
Ottmar

@ogobrecht
Copy link
Member

Oh, I think it should be:

  • function: create_row_column_defaults (parameter: p_enable_column_defaults boolean default false)
  • function: create_row_custom_defaults (parameter: p_custom_column_defaults xmltype or sys_refcursor? default null

Regards
Ottmar

@jgebal
Copy link
Contributor Author

jgebal commented May 28, 2017

I'm wondering it having 3 different API entry pints to create a row isn't over-compilcating stuff too much.
Ideally, one API should be enough. I wonder if there would be a scenario where you would like API to have all 3 and the consumers would use all three. Wouldn't it be enough to have just one (or two) and it's behavior would differ depending on how it's generated?
Also.
calling api: a_long_table_name_goes_here_api.create_row_column_defaults is a lot of typing.
Could we just say that:

  • create_row creates row with given columns
  • create_a_row creates a row with default values (unless overriden)
    I like short, easy to remember yet meaningful names.

I think that cursor could be better than XML as not all developers find XML easy enough.
Actually I was thinking of deriving the values from table itself (first row?) as a default behavior if generating API with defaults. The cursor could be an optional parameter.

@ogobrecht
Copy link
Member

Hi Jacek,

the parameter based entry point was to be able to direct assign APEX page items. The row type based for cases you want to change only some columns because it saves you much code and the procedure is useful if you don't care about a return value.

May be the procedure could be omitted and the column based could be parameterized? I have to change much APEX calls without the column based version. At the end the code is generated and you pick what you need. But I am open for new ideas... I can remember that André was also thinking about to have only row based functions. I have to discuss this with him.

Regarding the names - do you have an editor with autocompletion? ;-)

I find a clear long name better then a short one - but this is may be a personal preference. For users who only review the code the long name is definitely better. For a developer who works the whole day with the code a short name is better.

Is the longer name really an issue? Also I miss the distinction between the defaults defined for the table columns and the custom defaults. Do you only want to have the custom defaults?

What about create_row_defaults and create_row_custom_defaults? It's a bit shorter for the column defaults and clear enough regarding the distinction.

Best regards
Ottmar

@jgebal
Copy link
Contributor Author

jgebal commented May 28, 2017

Hmmm...
Seems like it would be great to have a chat typing takes so much time.
Do you guys are familiar with https://appear.in
We could schedule a meeting and chave a wideo conf call in a random meeting room.
I'd like to know a bit more about the problems you're solving with the generator and get to know if we actually aim towards something similar or are those two separate classes of problems that would require two different solutions.
It would be bad if you would try to make your library do too much it the "too much" is not simple generalization of specific problem.
Would you be willing to have a chat about it?

@ogobrecht
Copy link
Member

From my side no problem, best time for me would be something around 20:00 CET in the evening the next days. @aborngra: what do you think?

But you mentioned that you are currently traveling. So enjoy your time first and then we can have a chat.

@ogobrecht
Copy link
Member

Hi Jacek,

I discussed the open issues with André - as far as you agree with the following sum up wee don't need a chat.

We will see how long this will take - it is a free time project. We will keep you updated and create a new branch for the changes, so that you will be able to test before the official release.

Best regards (also from André)
Ottmar

Table API Generator - Next Steps

After some rethinking: All listed improvements should be backward compatible since we have more than 20 known and some unknown applications using the current functionality. For special use cases a developer can create a wrapper to set his personal defaults.

New Functionality

  • New parameter p_api_name varchar2 default null to control API name
    • Extended substitutions for the SQL Developer multi API generation like #TABLE_NAME_17# or better #TABLE_NAME_4_20# also for the parameter p_sequence_name
  • New method read_a_row without any parameter to read the first record from the table (one cursor fetch)
  • New method create_a_row with default parameters
    • If new parameter p_column_defaults xmltype default null is null then the table column defaults are used, otherwise the provided custom defaults
    • XMLTYPE because we need to save this parameter in the API spec for parameterless recreation and also this gives us the possibility to define some dynamic content like in the example below (the XML is very simple and since JSON is commonly used these days this xml should not be a problem for a developer - a helper function to convert a ref_curser into this xml would be nice)
    • Additional reason is the SQL Developer integration - there we have only a reduced set of input types
<defaults
  employee_id="employees_seq.nextval"
  first_name="'Chuck'"
  last_name="'Norris'"
  email="substr(sys_guid(),1,25)"
  phone_number="'123'"
  hire_date="to_date('2017-05-27','YYYY-MM-DD')"
  job_id="'FI_MGR'"
  salary="123"
  commission_pct="0.01"
  manager_id="null"
  department_id="null"
/>
  • New parameter enable_getter_and_setter boolean default true
  • New parameter return_row_instead_of_pk boolean default false
  • New parameter enable_parameter_prefixes boolean default true

Improvements

  • When using p_col_prefix_in_method_names => false then do NOT throw an exception when no unique column prefix is found for a table? To be discussed
  • Create tests with Travis and SonarQube
  • Something else?
  • ...

@jgebal
Copy link
Contributor Author

jgebal commented Jun 4, 2017

Hi @ogobrecht,
I didn't fully read through your code and I don't fully understand how it works (just know what the outcomes are ;) )
Keeping backward compatibility with existing solution seems like a good plan.

If you will create a separate branch for new features I might be able to do some skeleton for CI/CD on Travis and prepare basic unit testing for generator.
Having full suite of unit tests will definitely make it easier for you to work safely with the code,

Regards
Jacek

@ogobrecht
Copy link
Member

Hi Jacek, hi André,

here the current status from today:

Done

  • New Branch 0.5
  • New parameter p_api_name varchar2 default null to control API name
    • Extended substitutions for the SQL Developer multi API generation like #TABLE_NAME_17# or better #TABLE_NAME_4_20# also for the parameter p_sequence_name
    • Additional Info:
      • #TABLE_NAME_17# is treated as substr(1, 17)
      • #TABLE_NAME_4_20# is treated as substr(4, 20)
      • Negative positions also possible like #TABLE_NAME_-20_20# (length can NOT be ommitted because of use case with one number and backward compatibility, we had only #TABLE_NAME_26#, #TABLE_NAME_28#...)
  • New method read_a_row without any parameter to read the first record from the table (one cursor fetch)
  • New parameter enable_getter_and_setter boolean default true
  • New parameter enable_parameter_prefixes boolean default true

Work In Progress

  • New parameter return_row_instead_of_pk boolean default false
    • Parameter is already existing, but not working

To Do

  • New method create_a_row with default parameters
    • If new parameter p_column_defaults xmltype default null is null then the table column defaults are used, otherwise the provided custom defaults
    • XMLTYPE because we need to save this parameter in the API spec for parameterless recreation and also this gives us the possibility to define some dynamic content like in the example below (the XML is very simple and since JSON is commonly used these days this xml should not be a problem for a developer - a helper function to convert a ref_curser into this xml would be nice)
    • Additional reason is the SQL Developer integration - there we have only a reduced set of input types
  • When using p_col_prefix_in_method_names => false then do NOT throw an exception when no unique column prefix is found for a table? To be discussed
  • Align oddgen wrapper package for SQL Developer integration
  • Create tests with Travis and SonarQube
  • Something else?
  • ...

@ogobrecht
Copy link
Member

Hi Jacek,

the new parameter return_row_instead_of_pk boolean default false is now working.

The last missing feature is now "new method create_a_row with default parameters".

Best regards
Ottmar

@ogobrecht
Copy link
Member

Hi Jacek, hi André

now we are roughly feature complete - it was a short weekend ;-)

@jgebal: Please be so kind and do your first tests and let us know if you can live with current implementation of the column defaults. The two new helper functions to retrieve automatically the column_defaults are completely independent of the package and can be used as a base for own implementations and specific business needs.
The next steps from our side would be tests and documentation. Thank you again for your first test implementation. Could be, that we have to ask again for a little help when we start to implement more specific tests.

Best regards
Ottmar

Done - see also README.md in branch 0.5

  • New parameter enable_proc_with_out_params boolean default true: these procedures are created mainly for APEX to directly bind page items to the API without any other code - since not anyone is using APEX this can now be disabled
  • New method create_a_row with default parameters
    • If new parameter p_column_defaults xmltype default null is null then the table column defaults are used, otherwise the provided custom defaults
    • XMLTYPE because we need to save this parameter in the API spec for parameterless recreation and also this gives us the possibility to define some dynamic content like in the example below (the XML is very simple and since JSON is commonly used these days this xml should not be a problem for a developer
    • Additional reason is the SQL Developer integration - there we have only a reduced set of input types
    • Two new helper functions to retrieve automatically useful defaults:
      • om_tapigen.util_get_custom_col_defaults
      • om_tapigen.util_table_row_to_xml
      • The second one is used in the first one and the first one is a incomplete first implementation which could be used as a template for own business needs - please see also the spec and body for implementatione details; For the table EMPLOYEES the result works fine - see usage example below;
<defaults>
  <item><col>LAST_NAME</col><val>'Atkinson'</val></item>
  ...
</defaults>
-- usage of the helper om_tapigen.util_get_custom_col_defaults:
BEGIN
   om_tapigen.compile_api (
      p_table_name                    => 'EMPLOYEES',
      p_reuse_existing_api_params     => FALSE,
      p_col_prefix_in_method_names    => TRUE,
      p_enable_insertion_of_rows      => TRUE,
      p_enable_update_of_rows         => TRUE,
      p_enable_deletion_of_rows       => FALSE,
      p_enable_generic_change_log     => TRUE,
      p_enable_dml_view               => TRUE,
      p_sequence_name                 => 'EMPLOYEES_SEQ',
      p_api_name                      => 'EMPLOYEES_API',
      p_enable_getter_and_setter      => FALSE,
      p_enable_proc_with_out_params   => FALSE,
      P_enable_parameter_prefixes     => FALSE,
      p_return_row_instead_of_pk      => TRUE,
      p_column_defaults               => om_tapigen.util_get_custom_col_defaults ('EMPLOYEES'));
END;
/

-- inspect the result of the helper
SELECT XMLSERIALIZE (
          DOCUMENT OM_TAPIGEN.UTIL_GET_CUSTOM_COL_DEFAULTS (
                      P_TABLE_NAME   => 'CAT_FEE_TYPES')
          INDENT)
  FROM DUAL;

Remaining ToDo's

  • When using p_col_prefix_in_method_names => false then do NOT throw an exception when no unique column prefix is found for a table? To be discussed
  • Align oddgen wrapper package for SQL Developer integration
  • Update documentation
  • Create tests with Travis and SonarQube
  • Something else?
  • ...

@jgebal
Copy link
Contributor Author

jgebal commented Jun 20, 2017

Hi @ogobrecht. Awesome progress in a short period of time. I really appreciate.
I'll have a look at the generator and see how it would fit as a helper functionality for utPLSQL Unit Testing.
It looks like I should start blogging about it once I put all the pieces of the puzzle together :)

I can help with implementation of some unit tests.

The best way to start is to come up with requirements in form of small examples and turn those individual examples into tests.
I can have a look at one of simple functionalities and try to reverse-engineer the requirements and examples out of it.
I cannot commit on any time to deliver as I have my everyday job and utPLSQL project (still in quite active development).

@ogobrecht
Copy link
Member

Hi Jacek,

welcome to the club - the API generator is also a free time project of André and me :-)

It takes as long as it takes - no expectations from our side. We have also much to do with the documentation, the SQL Developer integration, the tests with utPLSQL and so on...

I had to fix today the pk column handling in the new method create_a_row - I was too concentrated to solve the issue with the parameter handling for the column defaults that I totally forgot the pk column handling: The pk column parameter defaults now to NULL since the sequence is handled by the create_row method or a trigger or in 12c automatically with the identity column and also configurable with the sequence name parameter.

now you should be able to create a row on the employees table with this simple call: employees_api.create_a_row;

Best regards
Ottmar

@jgebal
Copy link
Contributor Author

jgebal commented Jun 22, 2017

Hi @ogobrecht
Few issues found:

  • Data is not generated for TIMESTAMP datatype - this is a very common datatype and should be supported
  • Data is only generated in the get_a_row function for NOT NULL columns. I have a check constraint on two columns (column_a IS NOT NULL OR column_b IS NOT NULL). The default row is therefore unusable.
  • API cannot be generated for other user than the owner of the table, as USER_xxx data dictionary tables are used.

I would suggest:

  • support for TIMESTAMP datatype
  • generate row with defaults for all columns populated (not only NOT NULL)
  • add owner as a parameter for the generator so the API's can be generated for tables in other schemes. The API should be generated into schema owning the table (by default).

Thanks
Jacek

@ogobrecht
Copy link
Member

ogobrecht commented Jun 26, 2017

Hi Jacek,

here the changes from the weekend:

  • New Parameter p_owner all_users.username%type default user to be able to create APIs in other schemas
  • Rework pipelined table function view_existing_apis to be able to find also APIs with names other then <TABLE_NAME>_API since the API name is now changeable with the parameter p_api_name
  • Additional supported data types by the helper functions util_get_custom_col_defaults and util_table_row_to_xml - now the following data types should working: NUMBER, INTEGER, FLOAT, %CHAR%, DATE, TIMESTAMP and with dummy data CLOB, BLOB, XMLTYPE
  • Some small fixes and maintainability changes

I hope it is now working for your use case. The thing with the missing data types was clear. The weekend before I had not enough time and I wanted first to hear from you if the solution with the helper functions is generally ok for you, before I spend more time to improve it.

If someone needs special handling for the own business he/she can grab the functions and implement the special business logic in an additional helper package. I recommend also creating a wrapper function for the standard APIs in a company to be able to create APIs with a short one-liner.

Best regards
Ottmar

@jgebal
Copy link
Contributor Author

jgebal commented Jun 27, 2017

Thanks for the update. I will give it a try and let you know.

Best regards
Jacek

@ogobrecht
Copy link
Member

Hi Jacek,

the new version 0.5.0 is out - it took a long time...

The relevant parameter for your testing functionality is named p_enable_custom_defaults- please have a look in the docs for this parameter.

Hope you are able now to use it as expected for your tests. And sorry for the long time it took to the version 0.5.0, the changelog for this version is huge...

We have currently only simple, SQL script based tests implemented. The next steps are utPLSQL tests, set operations and logger integration.

Please let us know any comments or questions and please close this issue, if you think your requirements are met.

Best regards
Ottmar

@ogobrecht
Copy link
Member

Hi Jacek,

the new version 0.6.0 is out. I implemented some improvements for the dummy data generation when you set the parameter p_enable_custom_defaults to true (generated methods create_a_row, read_a_row, get_a_row). Please have a look in the parameter docs.

Since we have now bulk processing this might also interesting for you: bulk processing performance (we use here also your requested function get_a_row...)

Please let us know, if we can now close this issue or close it by yourself.

Best regards
Ottmar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants