SAP / abap-file-formats

File formats that define and specify the file representation for ABAP development objects
MIT License
58 stars 56 forks source link

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

Closed BeckerWdf closed 2 years ago

BeckerWdf commented 3 years ago

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 commented 3 years ago

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 https://github.com/SAP/abap-file-formats/blob/645506ae3941366b5f0436fe71537ba00faca244/.editorconfig#L14

larshp commented 2 years ago
  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 commented 2 years ago
  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 commented 2 years ago

why?

its a generic abap-file-format discussion, I think collecting the metadata of each object into one file 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

larshp commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

BeckerWdf commented 2 years ago

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.