Copilot commented on code in PR #149: URL: https://github.com/apache/fineract-consumer-facing/pull/149#discussion_r4178919173
########## consumer/src/test/java/org/apache/fineract/consumer/infrastructure/correlation/service/CorrelationIdHolderTest.java: ########## @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fineract.consumer.infrastructure.correlation.service; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; + +class CorrelationIdHolderTest { + + private final CorrelationIdHolder holder = new CorrelationIdHolder(); Review Comment: This test class also writes the static ThreadLocal in getReturnsStoredCorrelationIdAfterSet() without clearing it. If getReturnsNullBeforeSet() runs afterward, its newly constructed holder still observes the prior test's value, so the suite becomes order-dependent. Add an `@AfterEach` holder.clear() cleanup (or remove the static holder state). ########## consumer/src/main/java/org/apache/fineract/consumer/infrastructure/audit/data/NonTransactionalAuditEvent.java: ########## @@ -39,10 +40,19 @@ public final class NonTransactionalAuditEvent { private final boolean unknownPrincipal; private final String deviceFingerprint; private final Map<String, Object> details; + private final String correlationId; public static NonTransactionalAuditEvent of(AuditEventType eventType, Long userId, boolean unknownPrincipal, String deviceFingerprint, Map<String, Object> details) { + return of(eventType, userId, unknownPrincipal, deviceFingerprint, details, + CorrelationIdHolder.getCurrentCorrelationId()); Review Comment: Capturing from this `ThreadLocal` only works when the event is published on the request thread. `UserCommandServiceImpl.forgotPassword` publishes `PASSWORD_CHANGE_INITIATED` inside `applicationTaskExecutor.execute`, so this factory sees `null` and the audit row loses the originating request's correlation ID. Capture the ID before scheduling and pass it into the async work/event, or propagate the context through the executor. ########## consumer/src/test/java/org/apache/fineract/consumer/infrastructure/fineractclient/interceptors/FineractCorrelationIdInterceptorTest.java: ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fineract.consumer.infrastructure.fineractclient.interceptors; + +import static org.assertj.core.api.Assertions.assertThat; + +import feign.Request; +import feign.RequestTemplate; +import org.apache.fineract.consumer.infrastructure.correlation.service.CorrelationIdHolder; +import org.apache.fineract.consumer.infrastructure.fineractclient.data.FineractHeaders; +import org.junit.jupiter.api.Test; + +class FineractCorrelationIdInterceptorTest { + + private static final String TEST_CORRELATION_ID = "c6b8c9d1-0000-4000-8000-000000000001"; + + private final CorrelationIdHolder holder = new CorrelationIdHolder(); + private final FineractCorrelationIdInterceptor interceptor = new FineractCorrelationIdInterceptor(holder); Review Comment: These tests mutate CorrelationIdHolder.CURRENT through holder.set() but never clear it after each test. Because CURRENT is static, a populated test can leave its ID for doesNotSetHeaderWhenHolderIsEmpty(), making the result order-dependent and potentially causing an empty-holder test to send the previous test's header. Add an `@AfterEach` cleanup that calls holder.clear(). ########## frontend/src/app/core/interceptors/correlation-id.interceptor.ts: ########## @@ -0,0 +1,47 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { HttpInterceptorFn } from '@angular/common/http'; + +export function generateCorrelationId(): string { + if (typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function') { + return crypto.randomUUID(); + } + return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, (c) => { + const r = (Math.random() * 16) | 0; + const v = c === 'x' ? r : (r & 0x3) | 0x8; + return v.toString(16); + }); +} + +export const correlationIdInterceptor: HttpInterceptorFn = (req, next) => { + if (!req.url.startsWith('/api')) { + return next(req); Review Comment: This interceptor only covers Angular `HttpClient` requests, but `AuditService.flush()` sends `/api/v1/audit/events` with native `fetch` (frontend/src/app/core/audit/audit.service.ts:67-81), which bypasses every `HttpInterceptorFn`. Consequently client audit batches never carry `X-Correlation-ID` and the backend generates a different ID for them, so the audit event cannot be correlated with the request/error that caused it. Add the correlation header to that fetch path or route the audit request through `HttpClient`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
