feat(bpdm-gate): Adding a externalSequenceTimestamp attribute to the business partner input re…

[kunyao-cofinity-x]

…quest

Description

The BPDM gate requires functionality to handle data updates from the customer side based on timestamps. This feature ensures that the data is only updated if the new data from the customer has a more recent timestamp than the existing data.

eclipse-tractusx/sig-release#726

• Customer sends data to the gate system with a timestamp attribute (currentness).
• The gate system receives the data and invokes the GR process.
• The GR process compares the received timestamp with the stored timestamp.
• If the received timestamp is newer, the data is updated, and a success message is logged and sent to the customer.
• If the received timestamp is not newer, the update is rejected, and an informative message is logged and sent to the customer.

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for a committer review:

• Self-testing on local environment
• Test cases creation

[SujitMBRDI]

In general, Instant type can be used instead of Timestamp for better performance and modern API advantages, we can add DbTimestamp wrapper class to ensure truncation to microseconds and Also, refactor mapping and service function for improved readability and efficiency to ensure proper exception handling and null safety throughout the code

[Sebastian-Wurm]

Let's not mix the technical problem of messages / versions of the same business partner arriving in a wrong order with the currentness of the business partner. Currentness information has business value to it and must be added anyway sooner or later to the Gate as it is already available in the Pool. It may well be, that the currentness of a business partner needs to be changed, because currentness information was wrong. If the corrected currentness lies before the wrong currentness, this change would not be possible with this code. Let's remove this code here and rather introduce an integer version number at a business partner or a sequence number at the message level in another pull request. This version / sequence number could be optional for those source systems, that handle sequence problems on their own. If it is set, it must be incremented on each change of the business partner / sending of a message. Source systems not having an integer version / sequence number could rely on converting the timestamp to a unix time. This is best practice in other MDM system for decades.

[Sebastian-Wurm]

Please see my comment regarding currentness being a business attribute which should not be abused to solve technical problems.

[nicoprow]

Currently, the project does not compile on the test stage as there are some errors with the test objects.

As a first step I would recommend to clean up your commit history and reach a state in whcih you have one new commit that is on top of the current main branch. Then you can debug.

At the moment your commit history shows merges and and it references an outdated commit that is behind the current main.

[SujitMBRDI]

@kunyao-cofinity-x, need to cross check on requirement as do we really need timestamp on output stage? Also even if we add then can you please streamline attribute name as externalSequenceTimestamp only as it will create confusion in future while reading DB table content.

[kunyao-cofinity-x]

Hello @SujitMBRDI, thanks for looking into it. This attribute will be only applied to the input stage. I have pushed the modification of the column name.

[SujitMBRDI]

Hello @kunyao-cofinity-x,
Cc: @nicoprow

Code contribution looks fine but kindly perform below formalities to make your contribution available to main.

• rebase to the newest state of main
• update openAPI documents in the docs/api folder
• Add a CHANGELOG entry with a reference to the issue/PR
• Squash commits to have only 1 commit

[kunyao-cofinity-x]

Hello @kunyao-cofinity-x, Cc: @nicoprow

Code contribution looks fine but kindly perform below formalities to make your contribution available to main.

• rebase to the newest state of main
• update openAPI documents in the docs/api folder
• Add a CHANGELOG entry with a reference to the issue/PR
• Squash commits to have only 1 commit

Hi @SujitMBRDI, the following points are done

• rebase to the newest state of main
• update openAPI documents in the docs/api folder
• Add a CHANGELOG entry with a reference to the issue/PR
• Squash commits to have only 1 commit
  cc : @nicoprow

[SujitMBRDI]

LGTM

[nicoprow]

Just found a problem upon close inspection with the UpsertResult. See below for more details.

The current behaviour prevents the record from being updated but still sets the sharing state to Initial even though the business partner data has not been updated

[nicoprow]

The upsert result should give the information whether the record did indeed change. Before there was only hasChanges as a criterion but now we have also shouldUpdate. In the UpsertResult we should therefore pass the result of both values.

[kunyao-cofinity-x]

1. extend a new attribute to UpsertResult
2. return UpsertResult(hasChanges, shouldUpdate, changeType, partnerToUpsert)
3. .takeIf { it.hadChanges || it.shouldUpdate || sharingState.sharingStateType == SharingStateType.Error }

[nicoprow]

I found a case when the logic does not work as expected:

1. Create and share abusiness partner with an external timestamp A
2. Watch it go to Pending State
3. Update the business partner with an earlier timestamp A-1
4. The record data is not updated as expected but
5. Look into the sharing state and see that it got into ready again even though the data has not been updated

This is because the check for the upserted data is wrong. I recommend to delete the two flags in upsertedData class and replace it with something like "hasBeenUpserted" or "dataHasChanged" to indicate that the data has been successfully changed. The check is the result of the two conditions hasChanges and shouldUpdate. Querying that flag then when determing whether to update the sharing state should lead to the right behaviour