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

DDLS: Make the .objectdependencies.json file of the DDLS object type prettier #163

Closed
BeckerWdf opened this issue Aug 13, 2021 · 22 comments · Fixed by #311
Closed

DDLS: Make the .objectdependencies.json file of the DDLS object type prettier #163

BeckerWdf opened this issue Aug 13, 2021 · 22 comments · Fixed by #311

Comments

@BeckerWdf
Copy link
Contributor

Currently this json file can be improved in different ways:

  1. it could be formatted (indented) like the other json files
  2. UPPERCASE names could be converted to camelCase
  3. a JSON schema should be provided
@albertmink
Copy link
Contributor

albertmink commented Aug 13, 2021

With regard to the formatting

The Github action says, that the file has been checked
https://github.com/SAP/abap-file-formats/runs/3321557726?check_suite_focus=true#step:4:203

Something goes wrong here, since the formatting should follow the definition here

@larshp larshp changed the title Make the .objectdependencies.json file of the DDLS object type prettier DDLS: Make the .objectdependencies.json file of the DDLS object type prettier Aug 13, 2021
@larshp
Copy link
Collaborator

larshp commented Jan 20, 2022

  1. alternatively add it into the main object json file, so there is just one json file and one schema for the object type

@BeckerWdf
Copy link
Contributor Author

  1. alternatively add it into the main object json file, so there is just one json file and one schema for the object type

We want to keep it separate.

@larshp
Copy link
Collaborator

larshp commented Feb 11, 2022

why?

its a generic abap-file-format discussion, I think collecting the metadata of each object into one file makes it easier,

  • for the developer to figure out what has changed
  • keeping track of possible schema changes, if the there are multiple files and multiple schemas, not having cross dependencies makes it easier

another thing, I can see this specific json file as redundant, https://github.com/SAP/abap-file-formats/blob/main/README.md#background-and-scope, the information is just replicating already existing information?

plus it is not something that the developer creates/maintains/is interested in

@BeckerWdf
Copy link
Contributor Author

BeckerWdf commented Feb 11, 2022

why?

Because they serve two different purposes. The data in the main file is something a developer (at least) partially want's to review.
The .objectdependencies.json contains only information that is needed for the activation in the target system. So for the pure code review use case this is is not relevant (and the user can simply skip / ignore it).
For the "transport" use-case it's absolutely needed.

@albertmink
Copy link
Contributor

This topic applies in a wider context to many object, see comment here, in particular the last sentence. Unfortunately, it is not an easy decision - yet ;)

@BeckerWdf
Copy link
Contributor Author

This topic applies in a wider context to many object, see comment here, in particular the last sentence. Unfortunately, it is not an easy decision - yet ;)

I don't understand your point. This file is not about translation.

@albertmink
Copy link
Contributor

albertmink commented Feb 11, 2022

my bad, too much translations transports in my head. Anyhow, there are files ( for transport #131 , translation #106 , documentation #130 and more to come) for which we need to find a solution of how to do a proper Abap file format.

@larshp
Copy link
Collaborator

larshp commented Feb 11, 2022

ref https://github.com/SAP/abap-file-formats/blob/main/README.md#background-and-scope

plus no obsolete or redundant information as well as no autogenerated content

from a developer perspective, I know the ABAP CDS syntax, I fire my my favorite editor = notepad, enter the CDS, save as object.ddls.acds, this should be it, no additional files required(unless the description for DDLS is required)?

transport and activation, that part is somewhat circumstantial. It would correspond to having to maintain a list of methods in a different file than the ABAP file containing a class implementation.

@BeckerWdf
Copy link
Contributor Author

transport and activation, that part is somewhat circumstantial. It would correspond to having to maintain a list of methods in a different file than the ABAP file containing a class implementation.

So we only focus on code review? Don't we want to transfer also a CDS model from one system to another?

@larshp
Copy link
Collaborator

larshp commented Feb 11, 2022

Its not mutually exclusive(assuming the objectdependencies contain redundant information), like for classes we will have the source code in git, not all the stuff that goes into the SEO* database tables, or the actual includes, these are generated from the source code on activation/transport into a system.

@BeckerWdf
Copy link
Contributor Author

Its not mutually exclusive(assuming the objectdependencies contain redundant information), like for classes we will have the source code in git, not all the stuff that goes into the SEO* database tables, or the actual includes, these are generated from the source code on activation/transport into a system.

It is not redundant. The objectdependencies contains information that is not part of the main json.

@larshp
Copy link
Collaborator

larshp commented Feb 11, 2022

okay, I'm not super familiar with the way CDS views work, can you give an example of data in the file that is not redundant?

@BeckerWdf
Copy link
Contributor Author

Think of the following example:

@AbapCatalog.viewEnhancementCategory: [#NONE]
@AccessControl.authorizationCheck: #CHECK
@EndUserText.label: 'ddd'
define view entity z_sflight
  as select from sflight
  association to scarr on sflight.carrid = scarr.carrid
{
  key carrid     as Carrid,
  key connid     as Connid,
  key fldate     as Fldate,
      price      as Price,
      currency   as Currency,
      planetype  as Planetype,
      seatsmax   as Seatsmax,
      seatsocc   as Seatsocc,
      paymentsum as Paymentsum,
      seatsmax_b as SeatsmaxB,
      seatsocc_b as SeatsoccB,
      seatsmax_f as SeatsmaxF,
      seatsocc_f as SeatsoccF,
      scarr      as myAssoc
}

and on top of this we have:

@AbapCatalog.viewEnhancementCategory: [#NONE]
@AccessControl.authorizationCheck: #CHECK
@EndUserText.label: 'dd'
define view entity z_view_on_zsflight
  as select from z_sflight
{
  key Carrid,
  key Connid,
  key Fldate,
      myAssoc.carrname
}

The .objectdependencies.json file of z_view_on_zsflight looks like this:

{
  "BASEINFO": {
    "FROM": ["Z_SFLIGHT","SCARR"],
    "ASSOCIATED":[],
    "BASE":[],
    "ANNO_REF":[],
    "SCALAR_FUNCTION":[],
    "VERSION":0,
    "ANNOREF_EVALUATION_ERROR":""
  }
}

Have a look at "SCARR" in "FROM". This information is not inside the source code of this view. But only in the source of z_sflight.
We now transport z_sflight, z_view_on_zsflight, sflight and scarr together in one transport. During the activation in the transport target system we have to know in which order the activation has to be performed. Out of this string we know that we have to activate Z_SFLIGHT and SCARR before we can activate z_view_on_zsflight

One could say this is a similar concept as we have with the "import" statement in java.

@larshp
Copy link
Collaborator

larshp commented Feb 14, 2022

heh, thats trouble 😄 then I assume, if SCARR is changed, and another layer of CDS is added, then it will impact he .objectdependencies.json file of z_view_on_zsflight? Ie. the developer does not do any changes to anything in z_view_on_zsflight, but changes only dependent objects will result in changes.

This gives a tight coupling? If one team maintains SCARR, they should be able to independently change it, without impacting consumers(changes in files)?

@BeckerWdf
Copy link
Contributor Author

If SCARR is replaced with a new CDS-view then z_sflight is changed (association to ... clause changes). Do you mean that?

@larshp
Copy link
Collaborator

larshp commented Feb 14, 2022

yea, something like that 😄 perhaps I should do some testing

anyhow, it sounds like trouble, information from z_sflight leaks into the objectdepencencies file from z_view_on_zsflight.

If there are two systems implementing z_sflight in different ways, then objectdepencencies will never be stable.

then instead of having abstractions, it becomes tightly coupled

@larshp
Copy link
Collaborator

larshp commented Feb 14, 2022

and yea, I'm ignoring how to transport and activate it, thats a technical exercise which can probably be solved. Like CAP CDS does not have these extra files.

There are a lot of ABAP developers, the goal should be to make it as simple as possible for the developers creating applications, ie. fewer files, less information => better.

@BeckerWdf
Copy link
Contributor Author

anyhow, it sounds like trouble, information from z_sflight leaks into the objectdepencencies file from z_view_on_zsflight.

after changing the association in z_sflight to
association to z_scarr on sflight.carrid = z_scarr.Carrid
the objectdepencencies file of z_view_on_zsflight contains
"FROM":["Z_SCARR","Z_SFLIGHT"]

So yet the behaviour is exactly as you describe it.

@BeckerWdf
Copy link
Contributor Author

and yea, I'm ignoring how to transport and activate it, thats a technical exercise which can probably be solved. Like CAP CDS does not have these extra files.

I think CAP CDS has the "import" statement for this.
And yes this dependencies file is complicated and has it's weaknesses. But that's simple how things are at the moment.

the goal should be to make it as simple as possible for the developers creating applications, ie. fewer files, less information => better.

Yes, we try to get better and maybe we find a solution where we can get rid of this information completely. But we are not there yet.

@BeckerWdf
Copy link
Contributor Author

after changing the association in z_sflight to association to z_scarr on sflight.carrid = z_scarr.Carrid the objectdepencencies file of z_view_on_zsflight contains "FROM":["Z_SCARR","Z_SFLIGHT"]

So yet the behaviour is exactly as you describe it.

The object dependency file of z_view_on_zsflight does change. But it does not get recorded to a transport request automatically. If you now only transport and activate the change of z_sflight in a transport target system you will have the situation that z_view_on_zsflight is already active in this system. The activation of z_sflight does detect that it needs to re-activate all the views that us it so z_view_on_zsflight will simple re-activated (after z_sflight was activated) and in this re-activation the dependency information get's updated.

But you are right we may run into trouble if we do the follwing

  • Create z_sflight and z_view_on_zsflight and export it to git.
  • Change z_sflight and export only this change to git.
  • Now import the git repo state into a "clean" system (where these two CDS views do not exist).

@BeckerWdf
Copy link
Contributor Author

After a chat with @schneidermic0 we decided that we currently don't see a real benefit of the objectdependencies.json file.
So we will remove this completely.
I will provide a PR for this removal.

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

Successfully merging a pull request may close this issue.

4 participants