Skip to content

Conversation

tnleeuw
Copy link
Contributor

@tnleeuw tnleeuw commented May 19, 2025

No description provided.

@tnleeuw tnleeuw self-assigned this May 19, 2025
@@ -36,12 +35,11 @@ public PipeLineResult processPipeLine(PipeLine pipeLine, String messageId, Messa
// Reset the PipeLineSession and store the message and its id in the session
if (messageId == null) {
// This should not be touched anymore
messageId = MessageUtils.generateMessageId();
log.error("messageId not set, creating synthetic id [{}]", messageId);
throw new PipeRunException(null, "Pipeline of adapter ["+ pipeLine.getAdapter().getName()+"] received null message id");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misschien dat 'unable to process message without messageID' handiger is?

@@ -68,7 +68,7 @@ public class MessageUtils {

public static final String JSON_TEMPLATE_VALUE_QUOTED = "{\"%s\": \"%s\"}";
public static final String JSON_TEMPLATE_VALUE_UNQUOTED = "{\"%s\": %s}";
public static final String GENERATED_MESSAGE_ID_PREFIX = "synthetic-message-id-";
public static final String FALLBACK_MESSAGE_ID_PREFIX = "fallback-message-id-";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ik weet niet hoe handig het is deze aan te passen, ik vrees dat dit hard in de larva scenarios staat en niet makkelijk geignored kan worden.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ik dacht dat die message IDs sowieso al genegeerd zouden worden omdat ze altijd anders zijn. We zullen zien. Als het breekt, draait ik het terug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Het lijkt geen probleem te zijn in de Larva scenarios maar ik ga hem toch anders noemen. "Fallback" klinkt me toch te negatief, alsof iemand vergeten was een msg-id mee te sturen, terwijl het eigenlijk een normale situatie is in geval van het aanroepen van een sub-adapter.

@tnleeuw tnleeuw marked this pull request as draft May 19, 2025 12:03
@tnleeuw tnleeuw marked this pull request as ready for review May 20, 2025 06:53
@tnleeuw tnleeuw requested review from nielsm5 and evandongen May 20, 2025 06:53
@github-project-automation github-project-automation bot moved this to In Progress in Frank!Framework May 20, 2025
@tnleeuw tnleeuw moved this from In Progress to Review in Frank!Framework May 20, 2025
@tnleeuw tnleeuw added this to the 9.2.0 milestone May 20, 2025
@@ -425,6 +426,7 @@ private static void setupChildSession(PipeLineSession session, ParameterValueLis
if (correlationId != null) {
childSession.put(PipeLineSession.CORRELATION_ID_KEY, correlationId);
}
childSession.put(PipeLineSession.MESSAGE_ID_KEY, MessageUtils.generateMessageId());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Men zou deze dus kunnen overschrijven net als het correlatie id, al staat dat expliciet er in met een != check en deze gaat via de putAll hier onder?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ik vind deze en die van de ibislocalsender nog een beetje raar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

De putAll is alleen voor de parameters.
Je zou inderdaad met een parameter het messageId en correlationId kunnen overschrijven -- of dat goed is of niet weet ik niet, maar dat was wel al mogelijk.

De reden dat ik hier nu zelf een message-id zet is ergens onderscheid te kunnen blijven maken tussen de gevallen waar we zelf een nieuwe aanroep doen en een daarom een message-id genereren, en de gevallen waar we zonder message-id worden aangeroepen.

(Zoals een ApiListener aanroep die binnenkomt zonder message-id. Dan wordt in de Receiver uiteindelijk een message-id gegenereerd met prefix fallback-message-id zodat zichtbaar is dat het extern geinitieerd is, maar er geen extern message-id is aangeleverd. Is het prefix FF-MSG dan is zichtbaar dat het bericht binnen het FF geinitieerd is).

Misschien dat dat onderscheid nergens nodig is, maar het leek mij dat het zinvol kon zijn bij het debuggen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Het is nu een pijnpunt dat wanneer een subadapter aangereopen wordt, in combinatie met een iterating pipe, dat het 2e bericht geflagged wordt als duplicaat omdat ze het zelfde id krijgen / overnemen uit de parent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nu met dit PR, of nu in master / latest stable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ik snap het pijnpunt niet helemaal omdat de senders die een subadapter aanroepen, volgens mij nu helemaal geen message-id meegeven in de session? Dus ik weet niet zeker waar het duplicate message id in zit.

Tenzij de IteratorPipe het message-id zelf als parameter zet.
Maar dan is dat een probleem in de pipeline-configuration.

Toch?

Wat mis ik, wat zie ik over het hoofd?

Copy link

@nielsm5 nielsm5 merged commit 4d78599 into master May 21, 2025
32 of 33 checks passed
@nielsm5 nielsm5 deleted the techdebt/MessageIdGenerationCodeImprovements branch May 21, 2025 07:26
@github-project-automation github-project-automation bot moved this from Review to Done in Frank!Framework May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants