Skip to content

Commit b6c1835

Browse files
currenjincurrenjin
committed
Fix missing taskId filter and incorrect IN clause in JDBC profiling query DAOs
In JDBCJFRDataQueryDAO and JDBCPprofDataQueryDAO, the taskId parameter was validated but never added to the SQL WHERE clause, causing all profiling data to be returned regardless of the task. Additionally, the IN clause for instanceIds used a single comma-joined string parameter instead of individual bind parameters, which always returned empty results when multiple instances were specified.
1 parent 8544af1 commit b6c1835

4 files changed

Lines changed: 221 additions & 7 deletions

File tree

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.sql.ResultSet;
3535
import java.util.ArrayList;
3636
import java.util.Base64;
37+
import java.util.Collections;
3738
import java.util.List;
3839

3940
@RequiredArgsConstructor
@@ -56,13 +57,16 @@ public List<JFRProfilingDataRecord> getByTaskIdAndInstancesAndEvent(String taskI
5657
.append(" where ").append(JDBCTableInstaller.TABLE_COLUMN).append(" = ?");
5758
condition.add(JFRProfilingDataRecord.INDEX_NAME);
5859

59-
sql.append(" and ").append(JFRProfilingDataRecord.EVENT_TYPE).append(" =? ");
60+
sql.append(" and ").append(JFRProfilingDataRecord.TASK_ID).append(" = ?");
61+
condition.add(taskId);
62+
63+
sql.append(" and ").append(JFRProfilingDataRecord.EVENT_TYPE).append(" = ?");
6064
condition.add(eventType);
6165

6266
if (CollectionUtils.isNotEmpty(instanceIds)) {
63-
sql.append(" and ").append(JFRProfilingDataRecord.INSTANCE_ID).append(" in (?) ");
64-
String joinedInstanceIds = String.join(",", instanceIds);
65-
condition.add(joinedInstanceIds);
67+
sql.append(" and ").append(JFRProfilingDataRecord.INSTANCE_ID)
68+
.append(" in (").append(String.join(",", Collections.nCopies(instanceIds.size(), "?"))).append(")");
69+
condition.addAll(instanceIds);
6670
}
6771

6872
results.addAll(

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.sql.ResultSet;
2424
import java.util.ArrayList;
2525
import java.util.Base64;
26+
import java.util.Collections;
2627
import java.util.List;
2728
import lombok.RequiredArgsConstructor;
2829
import lombok.SneakyThrows;
@@ -56,10 +57,13 @@ public List<PprofProfilingDataRecord> getByTaskIdAndInstances(String taskId,
5657
.append(" where ").append(JDBCTableInstaller.TABLE_COLUMN).append(" = ?");
5758
condition.add(PprofProfilingDataRecord.INDEX_NAME);
5859

60+
sql.append(" and ").append(PprofProfilingDataRecord.TASK_ID).append(" = ?");
61+
condition.add(taskId);
62+
5963
if (CollectionUtils.isNotEmpty(instanceIds)) {
60-
sql.append(" and ").append(PprofProfilingDataRecord.INSTANCE_ID).append(" in (?) ");
61-
String joinedInstanceIds = String.join(",", instanceIds);
62-
condition.add(joinedInstanceIds);
64+
sql.append(" and ").append(PprofProfilingDataRecord.INSTANCE_ID)
65+
.append(" in (").append(String.join(",", Collections.nCopies(instanceIds.size(), "?"))).append(")");
66+
condition.addAll(instanceIds);
6367
}
6468

6569
results.addAll(

‎oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/test/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/dao/JDBCJFRDataQueryDAOTest.java‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
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.profiling.asyncprofiler.storage.JFRProfilingDataRecord;
22+
import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
23+
import org.apache.skywalking.oap.server.storage.plugin.jdbc.common.TableHelper;
24+
import org.junit.jupiter.api.BeforeEach;
25+
import org.junit.jupiter.api.Test;
26+
import org.junit.jupiter.api.extension.ExtendWith;
27+
import org.mockito.Mock;
28+
import org.mockito.junit.jupiter.MockitoExtension;
29+
import org.mockito.junit.jupiter.MockitoSettings;
30+
import org.mockito.quality.Strictness;
31+
32+
import java.util.ArrayList;
33+
import java.util.Arrays;
34+
import java.util.Collections;
35+
import java.util.List;
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 JDBCJFRDataQueryDAOTest {
47+
48+
@Mock
49+
private JDBCClient jdbcClient;
50+
@Mock
51+
private TableHelper tableHelper;
52+
53+
private JDBCJFRDataQueryDAO dao;
54+
55+
@BeforeEach
56+
void setUp() {
57+
dao = new JDBCJFRDataQueryDAO(jdbcClient, tableHelper);
58+
}
59+
60+
@Test
61+
void getByTaskIdAndInstancesAndEvent_shouldFilterByTaskId() throws Exception {
62+
when(tableHelper.getTablesWithinTTL(JFRProfilingDataRecord.INDEX_NAME))
63+
.thenReturn(Collections.singletonList("jfr_profiling_data"));
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.getByTaskIdAndInstancesAndEvent("task1", Collections.emptyList(), "CPU");
75+
76+
assertThat(capturedSql.get()).contains(JFRProfilingDataRecord.TASK_ID + " = ?");
77+
assertThat(capturedParams.get()).contains("task1");
78+
}
79+
80+
@Test
81+
void getByTaskIdAndInstancesAndEvent_shouldFilterByInstanceIds() throws Exception {
82+
when(tableHelper.getTablesWithinTTL(JFRProfilingDataRecord.INDEX_NAME))
83+
.thenReturn(Collections.singletonList("jfr_profiling_data"));
84+
85+
final AtomicReference<String> capturedSql = new AtomicReference<>();
86+
doAnswer(invocation -> {
87+
capturedSql.set(invocation.getArgument(0));
88+
return new ArrayList<>();
89+
}).when(jdbcClient).executeQuery(anyString(), any(), any(Object[].class));
90+
91+
dao.getByTaskIdAndInstancesAndEvent("task1", Arrays.asList("inst1", "inst2", "inst3"), "CPU");
92+
93+
assertThat(capturedSql.get()).contains(JFRProfilingDataRecord.INSTANCE_ID + " in (?,?,?)");
94+
}
95+
96+
@Test
97+
void getByTaskIdAndInstancesAndEvent_shouldReturnEmptyListWhenTaskIdIsBlank() throws Exception {
98+
final List<JFRProfilingDataRecord> result =
99+
dao.getByTaskIdAndInstancesAndEvent("", Arrays.asList("inst1"), "CPU");
100+
101+
assertThat(result).isEmpty();
102+
}
103+
}

‎oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/test/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/dao/JDBCPprofDataQueryDAOTest.java‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
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.profiling.pprof.storage.PprofProfilingDataRecord;
22+
import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
23+
import org.apache.skywalking.oap.server.storage.plugin.jdbc.common.TableHelper;
24+
import org.junit.jupiter.api.BeforeEach;
25+
import org.junit.jupiter.api.Test;
26+
import org.junit.jupiter.api.extension.ExtendWith;
27+
import org.mockito.Mock;
28+
import org.mockito.junit.jupiter.MockitoExtension;
29+
import org.mockito.junit.jupiter.MockitoSettings;
30+
import org.mockito.quality.Strictness;
31+
32+
import java.util.ArrayList;
33+
import java.util.Arrays;
34+
import java.util.Collections;
35+
import java.util.List;
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 JDBCPprofDataQueryDAOTest {
47+
48+
@Mock
49+
private JDBCClient jdbcClient;
50+
@Mock
51+
private TableHelper tableHelper;
52+
53+
private JDBCPprofDataQueryDAO dao;
54+
55+
@BeforeEach
56+
void setUp() {
57+
dao = new JDBCPprofDataQueryDAO(jdbcClient, tableHelper);
58+
}
59+
60+
@Test
61+
void getByTaskIdAndInstances_shouldFilterByTaskId() throws Exception {
62+
when(tableHelper.getTablesWithinTTL(PprofProfilingDataRecord.INDEX_NAME))
63+
.thenReturn(Collections.singletonList("pprof_profiling_data"));
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.getByTaskIdAndInstances("task1", Collections.emptyList());
75+
76+
assertThat(capturedSql.get()).contains(PprofProfilingDataRecord.TASK_ID + " = ?");
77+
assertThat(capturedParams.get()).contains("task1");
78+
}
79+
80+
@Test
81+
void getByTaskIdAndInstances_shouldFilterByInstanceIds() throws Exception {
82+
when(tableHelper.getTablesWithinTTL(PprofProfilingDataRecord.INDEX_NAME))
83+
.thenReturn(Collections.singletonList("pprof_profiling_data"));
84+
85+
final AtomicReference<String> capturedSql = new AtomicReference<>();
86+
doAnswer(invocation -> {
87+
capturedSql.set(invocation.getArgument(0));
88+
return new ArrayList<>();
89+
}).when(jdbcClient).executeQuery(anyString(), any(), any(Object[].class));
90+
91+
dao.getByTaskIdAndInstances("task1", Arrays.asList("inst1", "inst2", "inst3"));
92+
93+
assertThat(capturedSql.get()).contains(PprofProfilingDataRecord.INSTANCE_ID + " in (?,?,?)");
94+
}
95+
96+
@Test
97+
void getByTaskIdAndInstances_shouldReturnEmptyListWhenTaskIdIsBlank() throws Exception {
98+
final List<PprofProfilingDataRecord> result =
99+
dao.getByTaskIdAndInstances("", Arrays.asList("inst1"));
100+
101+
assertThat(result).isEmpty();
102+
}
103+
}

0 commit comments

Comments
 (0)