abapGit / abapGit

Git client for ABAP
https://abapgit.org
MIT License
1.52k stars 533 forks source link

Structures (TABL) serialize locally created includes #7015

Open sbcgua opened 3 weeks ago

sbcgua commented 3 weeks ago
image

Is it intended? It should not - the local extension must stay local. A bug?

UPD:

image
mbtools commented 3 weeks ago

Sounds like a bug.

I can't find any documentation of "local extension".

https://help.sap.com/doc/abapdocu_latest_index_htm/latest/en-US/index.htm?file=abenddic_append_structures.htm

https://help.sap.com/doc/abapdocu_cp_index_htm/CLOUD/en-US/index.htm?file=abenddic_structures.htm

https://help.sap.com/docs/SAP_NETWEAVER_702/ff5206fc6c551014a1d28b076487e7df/908d7310b1af11d194f600a0c929b3c3.html?q=dictionary%20include&locale=en-US

How old is this feature?

sbcgua commented 3 weeks ago

It is an appended structure (sorry, maybe I used incorrect terminology). So the feature is super old :)

image

The ZEDE*** structure is in repo, the ZINT* is "custom", for the client

mbtools commented 3 weeks ago

If you want to pre-deliver the append structure, it should be empty (no fields) in your dev system and therefore empty in the repo.

You would add yystcd2 in your test system to simulate what a customer does.

Or if it's up to the customer, the append structure shouldn't be included at all in zede_....

sbcgua commented 3 weeks ago

We are diving into the implementation too much :) The essence is that the structure (whatever it is and it's purpose) is enhanceable and is a part of repo. While the extension itself (ZINT*) is not (and is a part of an external package). So it should not be syncronized.

P.S. some implementation notes. the ZEDE structure is indeed intended for enhancements specifically. Thus the main code should be aware of it (BADI, ALVs and etc). This structure is included in several types, including the non-DDIC ones. Thus the structure is non-empty (_DUMMY) and enhanceable. But anyway these are specific details. If it were a "normal" structure, the enhacement of it would result in the very same issue above.

sbcgua commented 3 weeks ago

But maybe it is trickier than it seems on the first glance...

Then B should be syncronized. While C should not. Hmmm ...

mbtools commented 3 weeks ago

In principle, we serialize objects like they are defined in the system.

In your case, A includes B (or C) and therefore it should be serialized this way. But B (or C) should not be included in the repo.

It's the same issue as using a data element that is not included in the package for a field of a table. The table definition will include the data element (referenced by the field), but the data element itself is not included. The repo is incomplete and pulling it will lead to activation errors.

Why do you include the append at all, when customers can define it themselves?

fabianlupa commented 3 weeks ago

Hmm, I also see this setup quite a bit. Even with the customer specific appends being tracked in the partner dev system and delivered only specifically to that customer by separate package and namespace/prefix. This has the advantage of being able to track all customer specific extensions centrally and taking them into account without logging into each customer system and having repair keys handed out.

I also would have expected the append to not be serialized if it's not included in a package that belongs to the repo. Both from the example above actually, if they aren't in a package belonging to the repo.

With a data element it's different in my opinion. There only the type is referenced, which is fine as it may be a dependency of the repo. Semantically the append works the other way around, it references the table / structure it is appended to. If you delete the append, the fields are gone and it's consistent. If you delete the data element, the field is still there, the type is just invalid. Technically it might work differently though.

sbcgua commented 3 weeks ago

In your case, A includes B (or C) and therefore it should be serialized this way ...

Not really. This is a customer enhancement, isn't it? Suppose (and most probably) I don't commit from customer's system. But I install updates. Which will remove the enhancement. Which is bad.

And let's not be distracted by the fact that, in my case, it is a dedicated structure. It is the same for a "normal" one. Let's say I'm SAP and deploy you the VBAP table. And you enhance it. And then I release a service pack. Will the enhancement be destroyed? No. I think we need the same ability in AG. It is a normal use-case.

mbtools commented 3 weeks ago

Pulling a table into a system where the table already has an append should keep the append in place. Please create a separate issue if this is not the case.

In the scenario here, the table (or structure) already has an append and is then serialized. As you can see in the screenshots above, the table references the append/include. It's listed in DD03L just like a data element dependency. Therefore, the table should be serialized with the included fields. If we don't do that, the repo will not match the system. This inconsistency can only cause problems. For example, code that references the included fields would run into syntax errors.

fabianlupa commented 3 weeks ago

Just to make sure, we are all talking specifically about appends? Those standalone repository objects with their own object catalog entry which allow for modification free adding fields to foreign dictionary structures and database tables that have extensibility enabled.

Because what you are saying @mbtools makes total sense to me for includes in database tables / structures as those are defined by the developer of that table / structure and in the table / structure, so in the same package, just like data element references. But it doesn't for appends.

Technically appends and includes are very related but from an end user perspective I would not understand how a diff generated by an append in a database table will not get resolved on pull ("should keep the append in place") while a diff generated by an include will. Isn't the diff supposed to be a preview on what will change on pull? Or is the diff supposed to display "what's in the system"?

In the scenario here, the table (or structure) already has an append and is then serialized.

Isn't it the same scenario? Whether I use abapGit in the customer system to update the repo where locally an append was created in a package not included in the repo or whether the append was created in the partner system in a separate package not in the repo and gets rolled out using transport / separate abapGit repo, in both cases system state is identical and in both cases serialization is used.

It's listed in DD03L just like a data element dependency. Therefore, the table should be serialized with the included fields.

Sounds to me like the append "leaks into" the DD03L entries of the foreign object. However, if that is a technical reason to not implement some special behaviour here, that's also valid, but then I would expect this to be documented somewhere in the abapGit documentation. As a user of appends it doesn't feel like you are editing the object you are appending because you are not in "edit mode" for that object and you are also not doing a modification. The append seems to have its own lifecycle and it seems to reference the database table / structure, not the other way around. Maybe that's just UI though.

For example, code that references the included fields would run into syntax errors.

To prevent that any code or development objects that reference the fields in the append are also placed in a different package from the repo (same as the append itself usually), so not rolled out without the append, or dynamically coded. If you use a separate prefix or namespace for the append name and appended fields (ZZ/YY / /.../) any violations are usually easily spotted.

Edit: I just looked for a repo with an append in abapGit-tests but didn't find any? To be clear: I would expect the field definitions of the append to be defined in the standalone append TABL object and the DD03L entries of the table / structure that's appended is redundant information. I would also expect the append TABL object to have a reference to the object it appends and therefore to fully store all the information needed on its own.

fabianlupa commented 3 weeks ago

Quickly created a repo just to be able to look at the objects. Can move it over to abapGit-tests if needed.

image

https://github.com/fabianlupa/append-test

mbtools commented 3 weeks ago

We're on the same page. Thanks for creating the repo. Let's move it to https://github.com/abapGit-tests/TABL_append

The repo looks correct. The append is part of the table definition but the append fields are not included XML. I don't have time right now to test. I assume deserializing will create both objects correctly i.e. the table will contain the appended fields.

But how does AG behave or how should it behave, if the append structure is not included in the repo (delete zagtestapp_append.tabl.xml)?

fabianlupa commented 3 weeks ago

We're on the same page. Thanks for creating the repo. Let's move it to https://github.com/abapGit-tests/TABL_append

:+1: Done

I assume deserializing will create both objects correctly i.e. the table will contain the appended fields.

I'll do some testing on how the current behaviour works.

But how does AG behave or how should it behave, if the append structure is not included in the repo (delete zagtestapp_append.tabl.xml)?

I would expect TABL ZAGTEST_APPEND to manage the lifecycle of the added fields. If that object is removed, the fields should be gone. If it gets pulled, the fields should be added. The appended table is a dependency of the append, so it should error on pull if the table hasn't been created before. If the append exists in the system in a package that is included in the repo but the repo does not include the append object, I would expect the append to be deleted on pull. If the append is outside of the repo package hierarchy however, I would expect it to stay.

Does that make sense?

sbcgua commented 3 weeks ago

But how does AG behave or how should it behave, if the append structure is not included in the repo (delete zagtestapp_append.tabl.xml)?

Suppose we have structure MAIN, and MAIN_APPEND which belong to the repo. MAIN_APPEND is appended to the main. There is also EXT_APPEND, which is appended by the customer and belongs to another package (and being an enhancement in it's essense). I would expect that:

MAIN
  F1
  F2
  ...
  APEEND MAIN_APPEND (in repo)
  YYF1
  YYF2
  ...
  APPEND EXT_APPEND (customer enhancement)
  ...

1) The diff of the MAIN is empty - so the fact of append is not displayed, at least in the diff. Although theoretically, it would be nice to see the fact that an object was enhanced (but certainly not the priority). 2) If MAIN and MAIN_APPEND are changed in remote and pulled, the AG should pull the changes and re-apply the EXT_APPEND. This is the perfect and the right approach. However, if it would warn that the append should be reapplied, it can be a temporary solution.

2A) The full automatic way may potentially be tricky. E.g. I append EXT_APPEND and then MAIN_APPEND2 which is a part of the repo. But I guess it is super rare and would be a bad practice anyway.

mbtools commented 3 weeks ago
  1. The table does contain more fields in the customer system. AG should display the diff in the customer system. Not showing that the table contains EXT_APPEND would be inconsistent (serialized definition <> system definition). Isn't this how AG works?
  2. Agree. That's the point of appends.
fabianlupa commented 3 weeks ago

The table does contain more fields in the customer system. AG should display the diff in the customer system. Not showing that the table contains EXT_APPEND would be inconsistent (serialized definition <> system definition). Isn't this how AG works?

In my opinion no. We shouldn't show any extension to any object that has its lifecycle managed by a dedicated different object (i. e. the techniques without modification). If you want to version the extensions, put these extension objects in another repo. EXT_APPEND could be part of a different repo and there you can see its state.

I don't know how we currently handle other extensible object types where such extensions have their own object. Domain fixed value appends? CDS extensions? Class enhancements?

Might also be worth getting some more opinions on the matter.

mbtools commented 3 weeks ago

I have done some tests and your assumption that the table is independent of the append is correct... almost. You can, for example, pull the table without the append from the test repo. The resulting table won't contain the reference to the append. When pulling the append afterward, the table will be enhanced as expected. From that point of view, we could remove the field .INCLU--AP in the dd03p section.

However, if the table and append are in the same repo, the order of import is not defined since the objects are both TABL (our known gap). For example, if AG imports the append first, then you get activation errors. Eventually, we can solve this by completing the implementation of get_deserialize_order .

And then there are indexes... You can create an index that contains fields of the table and the append. That creates a dependency in the other direction (see index_with_append branch). Pulling just the table leads to an activation error. In this case, you won't be able to skip the append (like includes).

For fixed domain values, we have a test: https://github.com/abapGit-tests/DOMA_append For other enhancements, there's usually an ENHO object. CDS has DDLX.

sbcgua commented 5 days ago

A stupid question: does sap always sort append alphabetically? In below screen, digits represent creation order of the appends ... and I cannot put ALIEN2 after INPKG ... while if I name it _X - it is positioned at the end

image

UPD (for overview)

image
mbtools commented 5 days ago

You can't (?) / shouldn't have any code that depends on the order. So does the order matter?

sbcgua commented 5 days ago

Yes, probably it doesn't. It's just not what I expected. But actually, it simplifies things. So appends are always at the end and always in alpha order.

Preliminary, the issue looks relatively easy to implement ... (speaking about structure only and letting alone indexes). Does "objects" have any access to the upper context to know if a certain object is a part of the repo?

mbtools commented 5 days ago

No. "object" instances have no context. Nor should they (or we block option for parallel processing). The dependencies (order) are controlled by zcl_abapgit_objects->deserialize and zcl_abapgit_file_deserialize:

https://github.com/abapGit/abapGit/blob/41619e6e4df241e074f7684cb9ec0c8b70fa43d5/src/objects/core/zcl_abapgit_file_deserialize.clas.abap#L167