Skip to content

Commit 86b6d62

Browse files
authored
Fix duplicate TABLE_COLUMN condition in JDBCMetadataQueryDAO.findEndpoint() (#13794)
`findEndpoint()` was adding the `TABLE_COLUMN = ?` WHERE condition twice and binding `EndpointTraffic.INDEX_NAME` as a parameter twice, due to a copy-paste error. The generated SQL was: ```sql select * from endpoint_traffic where table_name = ? and service_id = ? and table_name = ? ... ``` The second `table_name = ?` condition is redundant. All other methods in `JDBCMetadataQueryDAO` add the `TABLE_COLUMN` condition exactly once. The fix removes the duplicate condition and its corresponding parameter binding.
1 parent 1b66915 commit 86b6d62

File tree

3 files changed

+135
-2
lines changed

3 files changed

+135
-2
lines changed

docs/en/changes/changes.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
* Push `taskId` filter down to the storage layer in `IAsyncProfilerTaskLogQueryDAO`, removing in-memory filtering from `AsyncProfilerQueryService`.
1111
* Fix missing parentheses around OR conditions in `JDBCZipkinQueryDAO.getTraces()`, which caused the table filter to be bypassed for all but the first trace ID. Replaced with a proper `IN` clause.
1212
* Fix missing `and` keyword in `JDBCEBPFProfilingTaskDAO.getTaskRecord()` SQL query, which caused a syntax error on every invocation.
13+
* Fix duplicate `TABLE_COLUMN` condition in `JDBCMetadataQueryDAO.findEndpoint()`, which was binding the same parameter twice due to a copy-paste error.
1314

1415
#### UI
1516

oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/dao/JDBCMetadataQueryDAO.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,12 @@ public JDBCMetadataQueryDAO(JDBCClient jdbcClient, int metadataQueryMaxSize, Mod
7373
this.tableHelper = new TableHelper(moduleManager, jdbcClient);
7474
}
7575

76+
JDBCMetadataQueryDAO(JDBCClient jdbcClient, int metadataQueryMaxSize, TableHelper tableHelper) {
77+
this.jdbcClient = jdbcClient;
78+
this.metadataQueryMaxSize = metadataQueryMaxSize;
79+
this.tableHelper = tableHelper;
80+
}
81+
7682
@Override
7783
@SneakyThrows
7884
public List<Service> listServices() {
@@ -217,8 +223,6 @@ public List<Endpoint> findEndpoint(String keyword, String serviceId, int limit,
217223
condition.add(EndpointTraffic.INDEX_NAME);
218224
sql.append(" and ").append(EndpointTraffic.SERVICE_ID).append("=?");
219225
condition.add(serviceId);
220-
sql.append(" and ").append(JDBCTableInstaller.TABLE_COLUMN).append(" = ?");
221-
condition.add(EndpointTraffic.INDEX_NAME);
222226
if (!Strings.isNullOrEmpty(keyword)) {
223227
sql.append(" and ").append(EndpointTraffic.NAME).append(" like concat('%',?,'%') ");
224228
condition.add(keyword);
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*
17+
*/
18+
19+
package org.apache.skywalking.oap.server.storage.plugin.jdbc.common.dao;
20+
21+
import org.apache.skywalking.oap.server.core.analysis.manual.endpoint.EndpointTraffic;
22+
import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
23+
import org.apache.skywalking.oap.server.storage.plugin.jdbc.common.JDBCTableInstaller;
24+
import org.apache.skywalking.oap.server.storage.plugin.jdbc.common.TableHelper;
25+
import org.junit.jupiter.api.BeforeEach;
26+
import org.junit.jupiter.api.Test;
27+
import org.junit.jupiter.api.extension.ExtendWith;
28+
import org.mockito.Mock;
29+
import org.mockito.junit.jupiter.MockitoExtension;
30+
import org.mockito.junit.jupiter.MockitoSettings;
31+
import org.mockito.quality.Strictness;
32+
33+
import java.util.ArrayList;
34+
import java.util.Arrays;
35+
import java.util.Collections;
36+
import java.util.concurrent.atomic.AtomicReference;
37+
38+
import static org.assertj.core.api.Assertions.assertThat;
39+
import static org.mockito.ArgumentMatchers.any;
40+
import static org.mockito.ArgumentMatchers.anyString;
41+
import static org.mockito.Mockito.doAnswer;
42+
import static org.mockito.Mockito.when;
43+
44+
@ExtendWith(MockitoExtension.class)
45+
@MockitoSettings(strictness = Strictness.LENIENT)
46+
class JDBCMetadataQueryDAOTest {
47+
48+
@Mock
49+
private JDBCClient jdbcClient;
50+
@Mock
51+
private TableHelper tableHelper;
52+
53+
private JDBCMetadataQueryDAO dao;
54+
55+
@BeforeEach
56+
void setUp() {
57+
dao = new JDBCMetadataQueryDAO(jdbcClient, 100, tableHelper);
58+
}
59+
60+
@Test
61+
void findEndpoint_shouldContainTableColumnConditionOnlyOnce() throws Exception {
62+
when(tableHelper.getTablesWithinTTL(EndpointTraffic.INDEX_NAME))
63+
.thenReturn(Collections.singletonList("endpoint_traffic"));
64+
65+
final AtomicReference<String> capturedSql = new AtomicReference<>();
66+
final AtomicReference<Object[]> capturedParams = new AtomicReference<>();
67+
doAnswer(invocation -> {
68+
capturedSql.set(invocation.getArgument(0));
69+
final Object[] allArgs = invocation.getArguments();
70+
capturedParams.set(Arrays.copyOfRange(allArgs, 2, allArgs.length));
71+
return new ArrayList<>();
72+
}).when(jdbcClient).executeQuery(anyString(), any(), any(Object[].class));
73+
74+
dao.findEndpoint("keyword", "serviceId", 10, null);
75+
76+
final String sql = capturedSql.get();
77+
final long tableColumnCount = countOccurrences(sql, JDBCTableInstaller.TABLE_COLUMN + " = ?");
78+
assertThat(tableColumnCount)
79+
.as("TABLE_COLUMN condition should appear exactly once in WHERE clause")
80+
.isEqualTo(1);
81+
82+
final Object[] params = capturedParams.get();
83+
final long tableNameParamCount = countOccurrences(params, EndpointTraffic.INDEX_NAME);
84+
assertThat(tableNameParamCount)
85+
.as("EndpointTraffic.INDEX_NAME should be bound exactly once as a parameter")
86+
.isEqualTo(1);
87+
}
88+
89+
@Test
90+
void findEndpoint_shouldFilterByServiceId() throws Exception {
91+
when(tableHelper.getTablesWithinTTL(EndpointTraffic.INDEX_NAME))
92+
.thenReturn(Collections.singletonList("endpoint_traffic"));
93+
94+
final AtomicReference<String> capturedSql = new AtomicReference<>();
95+
final AtomicReference<Object[]> capturedParams = new AtomicReference<>();
96+
doAnswer(invocation -> {
97+
capturedSql.set(invocation.getArgument(0));
98+
final Object[] allArgs = invocation.getArguments();
99+
capturedParams.set(Arrays.copyOfRange(allArgs, 2, allArgs.length));
100+
return new ArrayList<>();
101+
}).when(jdbcClient).executeQuery(anyString(), any(), any(Object[].class));
102+
103+
dao.findEndpoint(null, "my-service", 10, null);
104+
105+
assertThat(capturedSql.get()).contains(EndpointTraffic.SERVICE_ID + "=?");
106+
assertThat(capturedParams.get()).contains("my-service");
107+
}
108+
109+
private long countOccurrences(final String text, final String target) {
110+
int count = 0;
111+
int index = 0;
112+
while ((index = text.indexOf(target, index)) != -1) {
113+
count++;
114+
index += target.length();
115+
}
116+
return count;
117+
}
118+
119+
private long countOccurrences(final Object[] array, final Object target) {
120+
long count = 0;
121+
for (final Object item : array) {
122+
if (target.equals(item)) {
123+
count++;
124+
}
125+
}
126+
return count;
127+
}
128+
}

0 commit comments

Comments
 (0)