mariuz / flamerobin

FlameRobin is a database administration tool for Firebird RDBMS. Our goal is to build a tool that is: lightweight (small footprint, fast execution) cross-platform (Linux, Windows, Mac OS X, FreeBSD) dependent only on other Open Source software
http://flamerobin.org
MIT License
216 stars 66 forks source link

Procedures input and output parameters do not support TYPE OF COLUMN #169

Closed luronumen closed 2 years ago

luronumen commented 3 years ago

ACTUAL RESULT

ALTER PROCEDURE USER_SEARCH ( ID BIGINT ) RETURNS ( USER_NAME VARCHAR(32), FIRST_NAME VARCHAR(32), LAST_NAME VARCHAR(32), SITE_ID BIGINT, DBA TYPE OF VISIBLE, KEEPALIVE_TIME TIMESTAMP, VISIBLE TYPE OF VISIBLE, FULL_NAME VARCHAR(100) )

EXPECTED RESULT

ALTER PROCEDURE USER_SEARCH ( ID TYPE OF COLUMN USERS.ID ) RETURNS ( USER_NAME TYPE OF COLUMN USERS.USER_NAME, FIRST_NAME TYPE OF COLUMN USERS.FIRST_NAME, LAST_NAME TYPE OF COLUMN USERS.LAST_NAME, SITE_ID TYPE OF COLUMN USERS.SITE_ID, DBA TYPE OF COLUMN USERS.DBA, KEEPALIVE_TIME TYPE OF COLUMN USERS.KEEPALIVE_TIME, VISIBLE OF COLUMN USERS.VISIBLE, FULL_NAME TYPE OF COLUMN USERS.FULL_NAME )

STEPS TO REPRODUCE THIS ISSUE 1- Create a Firebird 3.0 database with the following DDL instruction: FB30_DDL.txt

IMPORTANT NOTES For more details about the Use of Column Type in Declarations please check: https://firebirdsql.org/file/documentation/html/en/refdocs/fblangref25/firebird-25-language-reference.html#fblangref25-ddl-proc-paramscoltype

luronumen commented 3 years ago

Retest result on FlameRobin v0.9.3.7: FAILED!

ACTUAL RESULT

ALTER PROCEDURE USER_SEARCH ( ID BIGINT ) RETURNS ( USER_NAME VARCHAR(32), FIRST_NAME VARCHAR(32), LAST_NAME VARCHAR(32), SITE_ID BIGINT, DBA TYPE OF COLUMN USERS.DBA, KEEPALIVE_TIME TIMESTAMP, VISIBLE TYPE OF COLUMN USERS.VISIBLE, FULL_NAME VARCHAR(100) )

arvanus commented 3 years ago

@Jdochoa Expected:

create or alter function FN_NAME (
    FIELD1 MYDOMAN)
returns OTHERDOMAIN
as
declare variable variable ANOTHERDOMAIN;
begin
end

Actual:

create or alter function FN_NAME (
    FIELD1 TYPE OF COLUMN NUMERIC(14,0) )
RETURNS   OTHERDOMAIN
AS
declare variable type of ANOTHERDOMAIN;
BEGIN
END
Jdochoa commented 3 years ago

@Jdochoa Expected:

create or alter function FN_NAME (
    FIELD1 MYDOMAN)
returns OTHERDOMAIN
as
declare variable variable ANOTHERDOMAIN;
begin
end

Actual:

create or alter function FN_NAME (
    FIELD1 TYPE OF COLUMN NUMERIC(14,0) )
RETURNS   OTHERDOMAIN
AS
declare variable type of ANOTHERDOMAIN;
BEGIN
END

Hi @arvanus.

i can't reproduce it, do you have more information?

./jo

luronumen commented 3 years ago

@Jdochoa Expected:

create or alter function FN_NAME (
    FIELD1 MYDOMAN)
returns OTHERDOMAIN
as
declare variable variable ANOTHERDOMAIN;
begin
end

Actual:

create or alter function FN_NAME (
    FIELD1 TYPE OF COLUMN NUMERIC(14,0) )
RETURNS   OTHERDOMAIN
AS
declare variable type of ANOTHERDOMAIN;
BEGIN
END

Hi @arvanus

Using this flamerobin.7z I also NOT able to reproduce the issue that you mentioned. Which Firebird version are you using?

arvanus commented 3 years ago

Sorry, forgot to mention "type of", try with these:


SET TERM ^ ;

create or alter function DUMMY_FN (
    NEW_PARAM type of MYDOMAN)
returns integer
as
begin
  /* Function Text */
  return 1;
end^

SET TERM ; ^
luronumen commented 3 years ago

Sorry, forgot to mention "type of", try with these:


SET TERM ^ ;

create or alter function DUMMY_FN (
    NEW_PARAM type of MYDOMAN)
returns integer
as
begin
  /* Function Text */
  return 1;
end^

SET TERM ; ^

Hi @arvanus

I can now reproduce the issue that you mentioned.

Best regards, Luciano

Jdochoa commented 3 years ago

Sorry, forgot to mention "type of", try with these:


SET TERM ^ ;

create or alter function DUMMY_FN (
    NEW_PARAM type of MYDOMAN)
returns integer
as
begin
  /* Function Text */
  return 1;
end^

SET TERM ; ^

Fix it.

luronumen commented 3 years ago

Hi @Jdochoa

Could you generate a new build of FlameRobin so that I can help in validating the open issue?

Thanks in advance, Luciano

Jdochoa commented 3 years ago

Luciano

flamerobin.zip

./jo

arvanus commented 3 years ago

Looks Fixed to me

luronumen commented 3 years ago

Luciano

flamerobin.zip

./jo

Hi @Jdochoa

This FlameRobin version do not list the procedures Input/Output parameters:

FlameRobin

Jdochoa commented 3 years ago

Luciano

flamerobin.zip ./jo

Hi @Jdochoa

This FlameRobin version do not list the procedures Input/Output parameters:

FlameRobin

Hi @luronumen.

I can't reproduce it, please verify your preferences imagen

Or wait the next snapshot.

luronumen commented 3 years ago

Hi @Jdochoa

After set Show columns and parameters in tree this issue is NOT reproducible! Thank you very much!

Best regards, Luciano

luronumen commented 3 years ago

Hi @Jdochoa

Retest result on flamerobin.zip:

FlameRobin_Issue2

Jdochoa commented 3 years ago

Hi @Jdochoa

Retest result on flamerobin.zip:

* ISSUE 1: FIXED!

* ISSUE 2: It is still reproducible:

FlameRobin_Issue2

Hi @luronumen.

I'm not sure about this, Flamerobin behaves like this: In the preferences imagen

If Only Show Datatype is checked, show this. imagen

If Only Show domains is checked, show this. imagen

If Show Boot domain and Datatype is checked, show this imagen

In which scenario should I check type of?

./jo

luronumen commented 3 years ago

Hi @Jdochoa

For the fields of a procedure that are defined as a TYPE OF COLUMN instead of the traditional field types (varchar, int, float, etc) I think it makes more sense to always show < tablename > . < columnname > instead of the traditional types (varchar, int, float, etc.)

Based on this I believe that we should check if a field of a procedure is TYPE OF COLUMN in all three possibilities:

Does it make sense to you too?

Jdochoa commented 3 years ago

Hi @Jdochoa

For the fields of a procedure that are defined as a TYPE OF COLUMN instead of the traditional field types (varchar, int, float, etc) I think it makes more sense to always show < tablename > . < columnname > instead of the traditional types (varchar, int, float, etc.)

Based on this I believe that we should check if a field of a procedure is TYPE OF COLUMN in all three possibilities:

* Only show datatype

* Only show domain

* Show Both domain and datatype

Does it make sense to you too?

Hi @luronumen

It is modified so that instead of DataType, typeOf is displayed, please review at https://github.com/Jdochoa/flamerobin.git

./jo

luronumen commented 3 years ago

Hi @Jdochoa

Could you generate a new build of FlameRobin so that I can validating the fix?

Thanks in advance, Luciano

luronumen commented 3 years ago

Hi @Jdochoa

I got to retest using the build that you mentioned on #210

Is it possible to show only TABLE.COLUMN instead of TYPE OF COLUMN TABLE.COLUMN?

FlameRobin

luronumen commented 3 years ago

Hi @Jdochoa

The build that you mentioned on #210 is showing the following issue when I try to select the Functions Dependencies tab:

FlameRobin_FunctionsError

Jdochoa commented 3 years ago

Hi @Jdochoa

The build that you mentioned on #210 is showing the following issue when I try to select the Functions Dependencies tab:

FlameRobin_FunctionsError

Thank you @luronumen, Fix it.

./jo

Jdochoa commented 3 years ago

Hi @Jdochoa

I got to retest using the build that you mentioned on #210

Is it possible to show only TABLE.COLUMN instead of TYPE OF COLUMN TABLE.COLUMN?

FlameRobin

Hi @luronumen

It's possible, but not easy. we need some changes in the datatypes. You decide if waiting or rollback the change.

./jo

luronumen commented 3 years ago

Hi @Jdochoa

No problem for me to wait. As soon as you have a version that fixes all these issue, please share the build here so I can help you to retest it. Thank you very much for the great job you've done to modernize FlameRobin. Lets fix all the issues, complete support for Firebird 3.0, and then move on to Firebird 4.0.

Best Regards, Luciano

arvanus commented 3 years ago

Hello everyone I would create a new column "Type of", that receives a Boolean value (checkbox), like IBexpert does. What do you think?

luronumen commented 3 years ago

Hello everyone I would create a new column "Type of", that receives a Boolean value (checkbox), like IBexpert does. What do you think?

Hi @arvanus

Did you mean replace the true/false strings in the grid with a checkbox in the Boolean type representation? That would be wonderful!

Best Regards, Luciano

arvanus commented 3 years ago

I mean like this: image

luronumen commented 3 years ago

Now I understand @arvanus ! In this case I think it would be better to just put a column called Type of Column/Domain between the Type and Not Null columns. What do you think?

FlameRobinType

Jdochoa commented 3 years ago

Now I understand @arvanus ! In this case I think it would be better to just put a column called Type of Column/Domain between the Type and Not Null columns. What do you think?

FlameRobinType

Hi @arvanus do you explain the functionality?

./jo

arvanus commented 3 years ago

Sorry @Jdochoa, I didn't understand what you meant Today Flamerobin show the data in a different way comparing to IBExpert, FR simply shows the type(DOMAIN) together, while IB splits the data. I'd do the same as IB, splitting Type into 3 columns: Type, Type of, Domain/Column Another alternative, is to keep "Type" as is (without the text TYPE OF), and simply add a column "Type of" as a boolean between "parameter" and "Type"

luronumen commented 3 years ago

Hi @arvanus and @Jdochoa

What do you think about this two columns:

luronumen commented 3 years ago

Hi @Jdochoa

The following issue is happening when I try to delete a record:

FlameRobin_Delete

luronumen commented 2 years ago

Retest result on FlameRobin 0.9.3.12: PASS! Thank you very much for the fix!

Best Regards, Luciano