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

Reverse engineering existing database : triggers in its own folder #232

Closed
paulkatich opened this issue Apr 22, 2019 · 18 comments
Closed

Reverse engineering existing database : triggers in its own folder #232

paulkatich opened this issue Apr 22, 2019 · 18 comments

Comments

@paulkatich
Copy link

I am using the obevo cli for reverse engineering on oracle database. Currently the reveng tool puts all the triggers and tables in the same file. What is Obevo recommendation on this ? Is it possible to separate the triggers in to separate files by table ? We have a legacy database which lot of DDL for tables with indices. By separating triggers in a separate file will it cause any issues with changes ?

@paulkatich
Copy link
Author

A use case for our database is we have logon triggers which sets the session specific configs. So this dont belong to a table.

@shantstepanian
Copy link
Contributor

We do allow triggers in separate files in their own folder named /trigger. It is not enabled by default for all dbms types; I'll have to enable it for Oracle.

Assuming the trigger does belong to a schema or user, the existing logic should work for it. If not, then we need a little more work on it, but no problem to do
Btw, is the ask here the same as your last question in #219? If so, I'll close that ticket

@paulkatich
Copy link
Author

Yes same as in #219

shantstepanian added a commit that referenced this issue Apr 30, 2019
Oracle reverse-engineering corrections for indices and triggers, referring to issues #231 and #232
@shantstepanian
Copy link
Contributor

Fix committed - you can give this snapshot a try - https://oss.sonatype.org/content/repositories/snapshots/com/goldmansachs/obevo/obevo-cli/master-SNAPSHOT/obevo-cli-master-20190501.001845-17-dist.zip

(This is the same link as for your other ticket #231)

FYI on the trigger code - you may want to leverage the in-built tokens to specify the schema of your trigger, in case the Sql actually requires it. For an example, see this code:

https://github.com/goldmansachs/obevo/blob/master/obevo-db-impls/obevo-db-oracle/src/test/resources/platforms/oracle/example1/step1/schema1/trigger/TRIGGER1.sql

@paulkatich
Copy link
Author

We had good improvemnts. But looks like we missed a few.

EX:

//// CHANGE name=change0
CREATE TABLE FP_SALE_TYPES
( SALE_TYPE_ID VARCHAR2(20) NOT NULL ENABLE,
NAME VARCHAR2(30) NOT NULL ENABLE,
DEFAULT_OPEN_SALE_IND VARCHAR2(1) DEFAULT 'Y',
DESCRIPTION VARCHAR2(255) NOT NULL ENABLE,
CREATED_DTM DATE DEFAULT sysdate,
CREATED_BY VARCHAR2(30) DEFAULT user,
CHANGED_DTM DATE DEFAULT sysdate,
CHANGED_BY VARCHAR2(30) DEFAULT user,
CREATED_BY_LE_ID NUMBER(36,0),
CREATED_BY_OPERATING_AS_LE_ID NUMBER(36,0),
CREATED_BY_ORG_UNIT_LE_ID NUMBER(36,0),
CONSTRAINT STYP_PK PRIMARY KEY (SALE_TYPE_ID)
USING INDEX PCTFREE 10 INITRANS 2 MAXTRANS 255 COMPUTE STATISTICS
TABLESPACE XCOSX_SMALL_01 ENABLE,
CONSTRAINT AVCON_850217_DEFAU_000 CHECK (DEFAULT_OPEN_SALE_IND IN ('Y', 'N')) ENABLE
) SEGMENT CREATION IMMEDIATE
PCTFREE 10 PCTUSED 40 INITRANS 1 MAXTRANS 255
NOCOMPRESS LOGGING
TABLESPACE XCOSD
GO

//// CHANGE name=change1
CREATE OR REPLACE FORCE EDITIONABLE EDITIONING VIEW "ATLANTIS"."FP_SALE_TYPES#" ("SALE_TYPE_ID", "NAME", "DEFAULT_OPEN_SALE_IND", "DESCRIPTION", "CREATED_DTM", "CREATED_BY", "CHANGED_DTM", "CHANGED_BY", "CREATED_BY_LE_ID", "CREATED_BY_OPERATING_AS_LE_ID", "CREATED_BY_ORG_UNIT_LE_ID") AS
select SALE_TYPE_ID SALE_TYPE_ID, NAME NAME, DEFAULT_OPEN_SALE_IND DEFAULT_OPEN_SALE_IND, DESCRIPTION DESCRIPTION, CREATED_DTM CREATED_DTM, CREATED_BY CREATED_BY, CHANGED_DTM CHANGED_DTM, CHANGED_BY CHANGED_BY, CREATED_BY_LE_ID CREATED_BY_LE_ID, CREATED_BY_OPERATING_AS_LE_ID CREATED_BY_OPERATING_AS_LE_ID, CREATED_BY_ORG_UNIT_LE_ID CREATED_BY_ORG_UNIT_LE_ID from FP_SALE_TYPES
GO

//// CHANGE name=change2
CREATE OR REPLACE EDITIONABLE TRIGGER "ATLANTIS"."FP_SCOA_AUD_TRG"
BEFORE INSERT OR UPDATE

ON "ATLANTIS"."FP_SCORE_ANSWERS#" FOR EACH ROW
BEGIN

DECLARE
LOGON_RECORD FP_PKG_SEC_SECURITY.T_LOGON_RECORD;
BEGIN
/* populate auditing columns */
IF INSERTING THEN

  :NEW.CREATED_BY  := USER;
  :NEW.CHANGED_BY  := USER;
       /* If the date created has been specified as part of the INSERT
          then use that date for created by and changed by
          otherwise set the values for both columns to todays date
       */
  IF   :NEW.CREATED_DTM IS NOT NULL THEN
       :NEW.CHANGED_DTM := :NEW.CREATED_DTM;
  ELSE
       :NEW.CREATED_DTM := SYSDATE;
       :NEW.CHANGED_DTM := SYSDATE;
  END IF;
	/* the following code was added by SHOLLOWS to support     */
	/* the storing of additional auditing data for each record */
LOGON_RECORD := fp_pkg_sec_security.get_logon;
:NEW.CREATED_BY_LE_ID			:= LOGON_RECORD.logon_user_le_id;
:NEW.CREATED_BY_OPERATING_AS_LE_ID	:= LOGON_RECORD.operating_as_le_id;
:NEW.CREATED_BY_ORG_UNIT_LE_ID	:= LOGON_RECORD.Trans_Org_Unit_le_id;

ELSIF UPDATING THEN
:NEW.CHANGED_BY := USER;
IF :NEW.CHANGED_DTM = :OLD.CHANGED_DTM
OR :NEW.CHANGED_DTM IS NULL THEN
:NEW.CHANGED_DTM := SYSDATE;
END IF;
END IF;
END;
END FP_SCOA_AUD_TRG;

/
ALTER TRIGGER "ATLANTIS"."FP_SCOA_AUD_TRG" ENABLE
GO

//// CHANGE name=change3
CREATE OR REPLACE EDITIONABLE TRIGGER "ATLANTIS"."FP_SCOC_AUD_TRG"
BEFORE INSERT OR UPDATE

ON "ATLANTIS"."FP_SCORE_CHOICES#" FOR EACH ROW
BEGIN

DECLARE
LOGON_RECORD FP_PKG_SEC_SECURITY.T_LOGON_RECORD;
BEGIN
/* populate auditing columns */
IF INSERTING THEN

  :NEW.CREATED_BY  := USER;
  :NEW.CHANGED_BY  := USER;
       /* If the date created has been specified as part of the INSERT
          then use that date for created by and changed by
          otherwise set the values for both columns to todays date
       */
  IF   :NEW.CREATED_DTM IS NOT NULL THEN
       :NEW.CHANGED_DTM := :NEW.CREATED_DTM;
  ELSE
       :NEW.CREATED_DTM := SYSDATE;
       :NEW.CHANGED_DTM := SYSDATE;
  END IF;
	/* the following code was added by SHOLLOWS to support     */
	/* the storing of additional auditing data for each record */
LOGON_RECORD := fp_pkg_sec_security.get_logon;
:NEW.CREATED_BY_LE_ID			:= LOGON_RECORD.logon_user_le_id;
:NEW.CREATED_BY_OPERATING_AS_LE_ID	:= LOGON_RECORD.operating_as_le_id;
:NEW.CREATED_BY_ORG_UNIT_LE_ID	:= LOGON_RECORD.Trans_Org_Unit_le_id;

ELSIF UPDATING THEN
:NEW.CHANGED_BY := USER;
IF :NEW.CHANGED_DTM = :OLD.CHANGED_DTM
OR :NEW.CHANGED_DTM IS NULL THEN
:NEW.CHANGED_DTM := SYSDATE;
END IF;
END IF;
END;
END FP_SCOC_AUD_TRG;

/
ALTER TRIGGER "ATLANTIS"."FP_SCOC_AUD_TRG" ENABLE
GO

//// CHANGE INDEX name=STYP_PK
CREATE UNIQUE INDEX STYP_PK ON FP_SALE_TYPES (SALE_TYPE_ID)
PCTFREE 10 INITRANS 2 MAXTRANS 255 COMPUTE STATISTICS
TABLESPACE XCOSX_SMALL_01
GO

//// CHANGE name=change4
CREATE INDEX "ATLANTIS"."TED_IX1" ON OPEN_FEVE ("FINANCIAL_EVENT_ID")
PCTFREE 10 INITRANS 2 MAXTRANS 255 COMPUTE STATISTICS
TABLESPACE "XCOSD"
PARALLEL 4
GO

@shantstepanian
Copy link
Contributor

Is that all in the same reverse-engineered file? I'm surprised to see the trigger and the indexes in the table file

If it is not a problem, can you send over the interim/output.sql file that the reverse-engineering generates? That would be the ideal test case for me. However, in case that is not possible (e.g. if you have any sensitive content in that file), I should be able to work with the sample you provided here

@paulkatich
Copy link
Author

@shantstepanian can I email you ?

@shantstepanian
Copy link
Contributor

You can get my email from here - https://github.com/goldmansachs/obevo/blob/master/Obevo_Javasig.pdf

@paulkatich
Copy link
Author

@shantstepanian sorry. I thought I can email. Our info sec team has some issues with this. Can you please work with the example above.

@shantstepanian
Copy link
Contributor

Hi, please try this snapshot
https://github.com/goldmansachs/obevo/releases/download/untagged-c71181f1029ffb1193da/obevo-cli-master-SNAPSHOT-dist.zip

I added a minor fix around the view names that may be throwing things off; it may or may not work.

If it does not, please execute the NEWREVENG command with this flag added: -debugLogEnabled

This will add additional information to your output to help analyze what is going on.

Thanks for your patience with this as we iron out these issues. With the new debugLogEnabled feature, we hopefully won't need as many iterations like this to resolve issues

@paulkatich
Copy link
Author

paulkatich commented May 6, 2019

@shantstepanian I am getting file not found when I try to download the file.

@shantstepanian
Copy link
Contributor

Sorry, try this link - https://github.com/goldmansachs/obevo/releases/download/master-SNAPSHOT/obevo-cli-master-SNAPSHOT-dist.zip

It should also be available from the Releases page (master-SNAPSHOT)

@paulkatich
Copy link
Author

The link doesnt work. I went to releases pages. I only get the source code but not jars. Can you please provide a link with the cli jar files ?

@paulkatich
Copy link
Author

Got it. @shantstepanian I am still seeing few examples where the trigger is in a separate file. I added the debug enabled. Can I know where is the diagnostic information ? Is it in the output.sql in interim folder ?

@shantstepanian
Copy link
Contributor

Clarifying the requirement - I do expect that triggers should be in a separate folder separate from the table file (as per your original requirement around the login triggers not belonging to a table). This is how Obevo currently works (i.e. no distinguishing between triggers belonging to a table and triggers that don't). We have it as a separate file as it allows us to modify the trigger file in place (as of today, any //// CHANGE in a table file cannot be edited in place, which makes it more cumbersome to manage triggers there)

I would not rule out a future enhancement to allow triggers to be defined either way (e.g. co-located with the table file vs. in a separate file entirely, and to remove the constraint mentioned above). We can create this under a separate issue if you'd like, though it is a larger change and thus won't happen in the near-term.

In terms of the debug information, that shows up as a line like so within the file:

 -- DEBUG COMMENT: secondaryName=null; newPatternMatch=true; objectType=FUNCTION; originalObjectName=SP3

This information is more useful for the index issues you saw in your other ticket, where indices were not getting grouped to the correct file. I'll post more comments on that ticket

@shantstepanian
Copy link
Contributor

Just wanted to check if this is still an issue, after the rerun that you did in issue #231?

@shantstepanian
Copy link
Contributor

Closing this ticket. Let us know if you face further issues on this issue

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

No branches or pull requests

2 participants