Skip to content

Commit 214f6d1

Browse files
juliannguyen4juliannguyen4
authored
[CLIENT-4120] Fix filter expressions being ignored when passed to batch write, batch read, batch remove, and batch apply policies (#933)
[CLIENT-4121] Remove as_exp *exp_list parameter from pyobject_to_policy_*() helpers Several reasons why pyobject_to_policy_*()'s exp_list parameter is redundant: - pyobject_to_policy_*() only uses exp_list by passing a reference to it to as_exp_new_from_pyobject(), and assigns its new value to policy->filter_exp. (basically the same as using a local pointer variable) The same can be done by passing exp_list_p directly to as_exp_new_from_pyobject and dereferencing it to set policy->filter_exp. This also sets the pointer at exp_list_p as intended. - The methods calling pyobject_to_policy_*() either 1) pass in a pointer to a stack allocated as_exp object, or 2) pass in NULL, as an argument to exp_list. The calling method does not use the stack allocated as_exp object since pyobject_to_policy_*() already heap allocates an as_exp object and sets exp_list_p to point to it. And the caller already uses exp_list_p to clear the heap allocated as_exp object. Hence there's no reason to pass a reference to the stack as_exp object in the caller's frame. Case 2 causes the bug in CLIENT-4120 because pyobject_to_policy_*() will ignore the Python expressions if exp_list is NULL. New tests are targeting changes to batch_write.c
1 parent 55e76a6 commit 214f6d1

24 files changed

+180
-160
lines changed

‎src/include/policy.h‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ as_status pyobject_to_policy_apply(AerospikeClient *self, as_error *err,
219219
PyObject *py_policy, as_policy_apply *policy,
220220
as_policy_apply **policy_p,
221221
as_policy_apply *config_apply_policy,
222-
as_exp *exp_list, as_exp **exp_list_p);
222+
as_exp **exp_list_p);
223223

224224
typedef enum {
225225
SECOND_AS_POLICY_WRITE,
@@ -238,47 +238,48 @@ as_status pyobject_to_policy_query(AerospikeClient *self, as_error *err,
238238
PyObject *py_policy, as_policy_query *policy,
239239
as_policy_query **policy_p,
240240
as_policy_query *config_query_policy,
241-
as_exp *exp_list, as_exp **exp_list_p);
241+
as_exp **exp_list_p);
242242

243243
as_status pyobject_to_policy_read(AerospikeClient *self, as_error *err,
244244
PyObject *py_policy, as_policy_read *policy,
245245
as_policy_read **policy_p,
246246
as_policy_read *config_read_policy,
247-
as_exp *exp_list, as_exp **exp_list_p);
247+
as_exp **exp_list_p);
248248

249249
as_status pyobject_to_policy_remove(AerospikeClient *self, as_error *err,
250250
PyObject *py_policy,
251251
as_policy_remove *policy,
252252
as_policy_remove **policy_p,
253253
as_policy_remove *config_remove_policy,
254-
as_exp *exp_list, as_exp **exp_list_p);
254+
as_exp **exp_list_p);
255255

256256
// py_policy_also_supports_info_policy_fields only applies if self->validate_keys is true
257-
as_status pyobject_to_policy_scan(
258-
AerospikeClient *self, as_error *err, PyObject *py_policy,
259-
as_policy_scan *policy, as_policy_scan **policy_p,
260-
as_policy_scan *config_scan_policy, as_exp *exp_list, as_exp **exp_list_p,
261-
bool py_policy_also_supports_info_policy_fields);
257+
as_status
258+
pyobject_to_policy_scan(AerospikeClient *self, as_error *err,
259+
PyObject *py_policy, as_policy_scan *policy,
260+
as_policy_scan **policy_p,
261+
as_policy_scan *config_scan_policy, as_exp **exp_list_p,
262+
bool py_policy_also_supports_info_policy_fields);
262263

263264
// py_policy_also_supports_info_policy_fields only applies if self->validate_keys is true
264265
as_status pyobject_to_policy_write(
265266
AerospikeClient *self, as_error *err, PyObject *py_policy,
266267
as_policy_write *policy, as_policy_write **policy_p,
267-
as_policy_write *config_write_policy, as_exp *exp_list, as_exp **exp_list_p,
268+
as_policy_write *config_write_policy, as_exp **exp_list_p,
268269
bool py_policy_also_supports_info_policy_fields);
269270

270271
as_status pyobject_to_policy_operate(AerospikeClient *self, as_error *err,
271272
PyObject *py_policy,
272273
as_policy_operate *policy,
273274
as_policy_operate **policy_p,
274275
as_policy_operate *config_operate_policy,
275-
as_exp *exp_list, as_exp **exp_list_p);
276+
as_exp **exp_list_p);
276277

277278
as_status pyobject_to_policy_batch(AerospikeClient *self, as_error *err,
278279
PyObject *py_policy, as_policy_batch *policy,
279280
as_policy_batch **policy_p,
280281
as_policy_batch *config_batch_policy,
281-
as_exp *exp_list, as_exp **exp_list_p);
282+
as_exp **exp_list_p);
282283

283284
as_status pyobject_to_map_policy(as_error *err, PyObject *py_policy,
284285
as_map_policy *policy, bool validate_keys);
@@ -301,25 +302,24 @@ as_status pyobject_to_batch_write_policy(AerospikeClient *self, as_error *err,
301302
PyObject *py_policy,
302303
as_policy_batch_write *policy,
303304
as_policy_batch_write **policy_p,
304-
as_exp *exp_list, as_exp **exp_list_p);
305+
as_exp **exp_list_p);
305306

306307
as_status pyobject_to_batch_read_policy(AerospikeClient *self, as_error *err,
307308
PyObject *py_policy,
308309
as_policy_batch_read *policy,
309310
as_policy_batch_read **policy_p,
310-
as_exp *exp_list, as_exp **exp_list_p);
311+
as_exp **exp_list_p);
311312

312313
as_status pyobject_to_batch_apply_policy(AerospikeClient *self, as_error *err,
313314
PyObject *py_policy,
314315
as_policy_batch_apply *policy,
315316
as_policy_batch_apply **policy_p,
316-
as_exp *exp_list, as_exp **exp_list_p);
317+
as_exp **exp_list_p);
317318

318319
as_status pyobject_to_batch_remove_policy(AerospikeClient *self, as_error *err,
319320
PyObject *py_policy,
320321
as_policy_batch_remove *policy,
321322
as_policy_batch_remove **policy_p,
322-
as_exp *exp_list,
323323
as_exp **exp_list_p);
324324

325325
// metrics_policy must be declared already

‎src/main/client/apply.c‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ PyObject *AerospikeClient_Apply_Invoke(AerospikeClient *self, PyObject *py_key,
6464
PyObject *py_ufunction = NULL;
6565

6666
// For converting expressions.
67-
as_exp exp_list;
6867
as_exp *exp_list_p = NULL;
6968

7069
as_static_pool static_pool;
@@ -111,7 +110,7 @@ PyObject *AerospikeClient_Apply_Invoke(AerospikeClient *self, PyObject *py_key,
111110
// Convert python policy object to as_policy_apply
112111
pyobject_to_policy_apply(self, &err, py_policy, &apply_policy,
113112
&apply_policy_p, &self->as->config.policies.apply,
114-
&exp_list, &exp_list_p);
113+
&exp_list_p);
115114
if (err.code != AEROSPIKE_OK) {
116115
goto CLEANUP;
117116
}

‎src/main/client/batch_apply.c‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -123,10 +123,7 @@ static PyObject *AerospikeClient_Batch_Apply_Invoke(
123123
as_batch_init(&batch, 0);
124124

125125
// For expressions conversion.
126-
as_exp batch_exp_list;
127126
as_exp *batch_exp_list_p = NULL;
128-
129-
as_exp batch_apply_exp_list;
130127
as_exp *batch_apply_exp_list_p = NULL;
131128

132129
PyObject *br_instance = NULL;
@@ -179,18 +176,18 @@ static PyObject *AerospikeClient_Batch_Apply_Invoke(
179176
sizeof(as_key) * processed_key_count);
180177

181178
if (py_policy_batch) {
182-
if (pyobject_to_policy_batch(
183-
self, err, py_policy_batch, &policy_batch, &policy_batch_p,
184-
&self->as->config.policies.batch, &batch_exp_list,
185-
&batch_exp_list_p) != AEROSPIKE_OK) {
179+
if (pyobject_to_policy_batch(self, err, py_policy_batch, &policy_batch,
180+
&policy_batch_p,
181+
&self->as->config.policies.batch,
182+
&batch_exp_list_p) != AEROSPIKE_OK) {
186183
goto CLEANUP;
187184
}
188185
}
189186

190187
if (py_policy_batch_apply) {
191188
if (pyobject_to_batch_apply_policy(
192189
self, err, py_policy_batch_apply, &policy_batch_apply,
193-
&policy_batch_apply_p, &batch_apply_exp_list,
190+
&policy_batch_apply_p,
194191
&batch_apply_exp_list_p) != AEROSPIKE_OK) {
195192
goto CLEANUP;
196193
}

‎src/main/client/batch_operate.c‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -130,10 +130,8 @@ static PyObject *AerospikeClient_Batch_Operate_Invoke(
130130
as_batch_init(&batch, 0);
131131

132132
// For expressions conversion.
133-
as_exp batch_exp_list;
134133
as_exp *batch_exp_list_p = NULL;
135134

136-
as_exp batch_write_exp_list;
137135
as_exp *batch_write_exp_list_p = NULL;
138136

139137
as_vector *unicodeStrVector = as_vector_create(sizeof(char *), 128);
@@ -208,18 +206,18 @@ static PyObject *AerospikeClient_Batch_Operate_Invoke(
208206
sizeof(as_key) * processed_key_count);
209207

210208
if (py_policy_batch) {
211-
if (pyobject_to_policy_batch(
212-
self, err, py_policy_batch, &policy_batch, &policy_batch_p,
213-
&self->as->config.policies.batch, &batch_exp_list,
214-
&batch_exp_list_p) != AEROSPIKE_OK) {
209+
if (pyobject_to_policy_batch(self, err, py_policy_batch, &policy_batch,
210+
&policy_batch_p,
211+
&self->as->config.policies.batch,
212+
&batch_exp_list_p) != AEROSPIKE_OK) {
215213
goto CLEANUP;
216214
}
217215
}
218216

219217
if (py_policy_batch_write) {
220218
if (pyobject_to_batch_write_policy(
221219
self, err, py_policy_batch_write, &policy_batch_write,
222-
&policy_batch_write_p, &batch_write_exp_list,
220+
&policy_batch_write_p,
223221
&batch_write_exp_list_p) != AEROSPIKE_OK) {
224222
goto CLEANUP;
225223
}

‎src/main/client/batch_read.c‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -154,14 +154,13 @@ PyObject *AerospikeClient_BatchRead(AerospikeClient *self, PyObject *args,
154154
as_policy_batch *policy_batch_p = NULL;
155155

156156
// For expressions conversion.
157-
as_exp batch_exp_list;
158157
as_exp *batch_exp_list_p = NULL;
159158

160159
if (py_policy_batch) {
161-
if (pyobject_to_policy_batch(
162-
self, &err, py_policy_batch, &policy_batch, &policy_batch_p,
163-
&self->as->config.policies.batch, &batch_exp_list,
164-
&batch_exp_list_p) != AEROSPIKE_OK) {
160+
if (pyobject_to_policy_batch(self, &err, py_policy_batch, &policy_batch,
161+
&policy_batch_p,
162+
&self->as->config.policies.batch,
163+
&batch_exp_list_p) != AEROSPIKE_OK) {
165164
goto CLEANUP3;
166165
}
167166
}

‎src/main/client/batch_remove.c‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -122,10 +122,8 @@ static PyObject *AerospikeClient_Batch_Remove_Invoke(
122122
as_batch_init(&batch, 0);
123123

124124
// For expressions conversion.
125-
as_exp batch_exp_list;
126125
as_exp *batch_exp_list_p = NULL;
127126

128-
as_exp batch_remove_exp_list;
129127
as_exp *batch_remove_exp_list_p = NULL;
130128

131129
PyObject *br_instance = NULL;
@@ -173,18 +171,18 @@ static PyObject *AerospikeClient_Batch_Remove_Invoke(
173171
sizeof(as_key) * processed_key_count);
174172

175173
if (py_policy_batch) {
176-
if (pyobject_to_policy_batch(
177-
self, err, py_policy_batch, &policy_batch, &policy_batch_p,
178-
&self->as->config.policies.batch, &batch_exp_list,
179-
&batch_exp_list_p) != AEROSPIKE_OK) {
174+
if (pyobject_to_policy_batch(self, err, py_policy_batch, &policy_batch,
175+
&policy_batch_p,
176+
&self->as->config.policies.batch,
177+
&batch_exp_list_p) != AEROSPIKE_OK) {
180178
goto CLEANUP;
181179
}
182180
}
183181

184182
if (py_policy_batch_remove) {
185183
if (pyobject_to_batch_remove_policy(
186184
self, err, py_policy_batch_remove, &policy_batch_remove,
187-
&policy_batch_remove_p, &batch_remove_exp_list,
185+
&policy_batch_remove_p,
188186
&batch_remove_exp_list_p) != AEROSPIKE_OK) {
189187
goto CLEANUP;
190188
}

‎src/main/client/batch_write.c‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,14 +44,12 @@
4444
PyObject *py___policy = \
4545
PyObject_GetAttrString(py_batch_record, FIELD_NAME_BATCH_POLICY); \
4646
if (py___policy != Py_None) { \
47-
as_exp *expr = NULL; \
48-
as_exp *expr_p = expr; \
47+
as_exp *expr_p = NULL; \
4948
if (py___policy != NULL) { \
5049
__policy = (__policy_type *)malloc(sizeof(__policy_type)); \
5150
garb->policy_to_free = __policy; \
5251
if (__conversion_func(self, err, py___policy, __policy, \
53-
&__policy, expr, \
54-
&expr_p) != AEROSPIKE_OK) { \
52+
&__policy, &expr_p) != AEROSPIKE_OK) { \
5553
/* Don't call strstr unless we have to. It is a linear time operation */ \
5654
/* Also, not bothering to use POSIX regex library in this case */ \
5755
if (!(self->validate_keys && \
@@ -135,7 +133,6 @@ static PyObject *AerospikeClient_BatchWriteInvoke(AerospikeClient *self,
135133

136134
as_policy_batch batch_policy;
137135
as_policy_batch *batch_policy_p = NULL;
138-
as_exp exp_list;
139136
as_exp *exp_list_p = NULL;
140137

141138
PyObject *py_batch_type = NULL;
@@ -171,7 +168,7 @@ static PyObject *AerospikeClient_BatchWriteInvoke(AerospikeClient *self,
171168
if (pyobject_to_policy_batch(self, err, py_policy, &batch_policy,
172169
&batch_policy_p,
173170
&self->as->config.policies.batch,
174-
&exp_list, &exp_list_p) != AEROSPIKE_OK) {
171+
&exp_list_p) != AEROSPIKE_OK) {
175172
goto CLEANUP4;
176173
}
177174
}

‎src/main/client/exists.c‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ extern PyObject *AerospikeClient_Exists_Invoke(AerospikeClient *self,
5353
as_record *rec = NULL;
5454

5555
// For converting expressions.
56-
as_exp exp_list;
5756
as_exp *exp_list_p = NULL;
5857

5958
// Initialisation flags
@@ -83,8 +82,7 @@ extern PyObject *AerospikeClient_Exists_Invoke(AerospikeClient *self,
8382

8483
// Convert python policy object to as_policy_exists
8584
pyobject_to_policy_read(self, &err, py_policy, &read_policy, &read_policy_p,
86-
&self->as->config.policies.read, &exp_list,
87-
&exp_list_p);
85+
&self->as->config.policies.read, &exp_list_p);
8886
if (err.code != AEROSPIKE_OK) {
8987
goto CLEANUP;
9088
}

‎src/main/client/get.c‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ PyObject *AerospikeClient_Get_Invoke(AerospikeClient *self, PyObject *py_key,
5454
as_record *rec = NULL;
5555

5656
// For converting expressions.
57-
as_exp exp_list;
5857
as_exp *exp_list_p = NULL;
5958

6059
// Initialised flags
@@ -85,8 +84,7 @@ PyObject *AerospikeClient_Get_Invoke(AerospikeClient *self, PyObject *py_key,
8584

8685
// Convert python policy object to as_policy_exists
8786
pyobject_to_policy_read(self, &err, py_policy, &read_policy, &read_policy_p,
88-
&self->as->config.policies.read, &exp_list,
89-
&exp_list_p);
87+
&self->as->config.policies.read, &exp_list_p);
9088
if (err.code != AEROSPIKE_OK) {
9189
goto CLEANUP;
9290
}

‎src/main/client/operate.c‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -915,7 +915,6 @@ static PyObject *AerospikeClient_Operate_Invoke(AerospikeClient *self,
915915
as_policy_operate *operate_policy_p = NULL;
916916

917917
// For expressions conversion.
918-
as_exp exp_list;
919918
as_exp *exp_list_p = NULL;
920919

921920
as_vector *unicodeStrVector = as_vector_create(sizeof(char *), 128);
@@ -925,10 +924,10 @@ static PyObject *AerospikeClient_Operate_Invoke(AerospikeClient *self,
925924
as_operations_inita(&ops, size);
926925

927926
if (py_policy) {
928-
if (pyobject_to_policy_operate(
929-
self, err, py_policy, &operate_policy, &operate_policy_p,
930-
&self->as->config.policies.operate, &exp_list,
931-
&exp_list_p) != AEROSPIKE_OK) {
927+
if (pyobject_to_policy_operate(self, err, py_policy, &operate_policy,
928+
&operate_policy_p,
929+
&self->as->config.policies.operate,
930+
&exp_list_p) != AEROSPIKE_OK) {
932931
goto CLEANUP;
933932
}
934933
}
@@ -1090,7 +1089,6 @@ AerospikeClient_OperateOrdered_Invoke(AerospikeClient *self, as_error *err,
10901089
as_operations_inita(&ops, ops_list_size);
10911090

10921091
// For expressions conversion.
1093-
as_exp exp_list;
10941092
as_exp *exp_list_p = NULL;
10951093

10961094
/* These are the values which will be returned in a 3 element list */
@@ -1101,10 +1099,10 @@ AerospikeClient_OperateOrdered_Invoke(AerospikeClient *self, as_error *err,
11011099
CHECK_CONNECTED(err);
11021100

11031101
if (py_policy) {
1104-
if (pyobject_to_policy_operate(
1105-
self, err, py_policy, &operate_policy, &operate_policy_p,
1106-
&self->as->config.policies.operate, &exp_list,
1107-
&exp_list_p) != AEROSPIKE_OK) {
1102+
if (pyobject_to_policy_operate(self, err, py_policy, &operate_policy,
1103+
&operate_policy_p,
1104+
&self->as->config.policies.operate,
1105+
&exp_list_p) != AEROSPIKE_OK) {
11081106
goto CLEANUP;
11091107
}
11101108
}

0 commit comments

Comments
 (0)