feat(ffi): add FFI interface to get operators along with fields while performing expressions validation #263
Conversation
Oyami-Srk
changed the title
Oyami-Srk
force-pushed
the
KAG-5013
branch
2 times, most recently
from
cf1954c to
a009d30
Compare
Oyami-Srk
changed the title
src/ffi.rs
Outdated
| #[cfg(feature = "expr_validation")] | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn expression_validate( | ||
| atc: *const u8, |
There was a problem hiding this comment.
any reason to use u8 for strings here, and i8 elsewhere?
There was a problem hiding this comment.
According to the document, ffi::c_char is defined as i8. However, the document also mentioned that On modern architectures this type will always be either i8 or u8.
Choosing u8 here is just to keep consistency with Rust internal bytes represent like String::as_bytes.
P.S. I saw all the FFI interfaces in this project have no consistent way to represent c_char. E.g. fields is u8 in router_get_fields and i8 in context_add_value.
There was a problem hiding this comment.
@Oyami-Srk In general, we tries to use u8 for char whenever possible, except when dealing with char provided by LuaJIT.
This is because LuaJIT converts string to const char *, and this is a signed value. If we accept u8 LuaJIT will complain when attempting to convert a string.
Oyami-Srk
force-pushed
the
KAG-5013
branch
2 times, most recently
from
2820904 to
2490bba
Compare
|
Rebased with the latest main branch. Nothing else has been changed except |
…ong with fields
The FFI interface introducing by this commit has corresponding Go wrapping:
```
func ValidateExpression(atc string, s *Schema) (bool, []string, int64) {
atcC := unsafe.Pointer(C.CString(atc))
defer C.free(atcC)
errLen := C.ulong(1024)
errBuf := [1024]C.uchar{}
expr := C.expression_validate((*C.uchar)(atcC), s.s, &errBuf[0], &errLen)
defer C.expression_validate_free_result(expr)
if expr == nil {
fmt.Println("Error: ", string(errBuf[:errLen]))
return false, nil, 0
}
validate := bool(expr.validate)
operators := int64(expr.operators)
flds := make([]string, expr.fields_total)
flds_slice := unsafe.Slice(expr.fields, expr.fields_total)
for i := range flds {
flds[i] = C.GoString((*C.char)(unsafe.Pointer(flds_slice[i])))
}
return validate, flds, operators
}
```
…Rust
An example go binding for the FFI interface introduced by this commit:
```Go
func ValidateExpression(atc string, s *Schema) (bool, []string, uint64, error) {
atcC := unsafe.Pointer(C.CString(atc))
defer C.free(atcC)
errLen := C.ulong(1024)
errBuf := [1024]C.uchar{}
fieldsLen := C.ulong(1024)
fieldsBuf := [1024]C.uchar{}
fieldsTotal := C.ulong(0)
operatorsC := C.uint64_t(0)
result := C.expression_validate((*C.uchar)(atcC), s.s, &fieldsBuf[0], &fieldsLen, &fieldsTotal, &operatorsC, &errBuf[0], &errLen)
if bool(result) == false {
return false, nil, 0, fmt.Errorf(string(errBuf[:errLen]))
}
operators := uint64(operatorsC)
flds := make([]string, uintptr(fieldsTotal))
p := 0
for i := range flds {
flds[i] = C.GoString((*C.char)(unsafe.Pointer(&fieldsBuf[p])))
p += len(flds[i]) + 1
}
return true, flds, operators, nil
}
```
Co-authored-by: Javier <[email protected]>
|
@javierguerragiraldez Could you please approve this if there's no other concern in this Rust implementation? |
src/ffi.rs
Outdated
| /// - ATC_ROUTER_EXPRESSION_VALIDATE_FAILED(1) if validation is failed. | ||
| /// - ATC_ROUTER_EXPRESSION_VALIDATE_BUF_TOO_SMALL(2) if the provided fields buffer is not enough. | ||
| /// | ||
| /// If `fields_buf` is null and `fields_len` or `fields_total` is non-null, it will write |
There was a problem hiding this comment.
I think we should just be optimistic here and always ask the user to pass in a buffer, 99% of the time reallocate will not be necessary, and we can avoid having to parse the expression twice which could be expensive (especially with Regexes).
There was a problem hiding this comment.
I've removed the ability to acquire the buffer length. Callers must pass a valid buffer if the calling is for getting the fields in use.
But I keep the fields_buf null-able because this interface is expression_validate and callers may not always want fields. This will allow us to do more refactoring on the Kong side with this interface. Like getting rid of validating expressions by inserting and then deleting them.
…stead. This commit contains two parts: 1. Moving everything guarded by feature flag `expr_validation` to `ffi.rs`. 2. Remove feature flag `expr_validation`. 3. Remove public interfaces `get_field` and `get_operator` for `Predicate`. No internal logic has been changed.
remove the redundant error message when buffer is too small. remove the ability to fetch fields count and total length without a valid `field_buf`. Must pass a valid `field_buf` if calling for getting fields.
|
Did a little improvement:
@dndx Could you review it when you have a moment? |
👍 done |
Co-authored-by: Datong Sun <[email protected]>
Co-authored-by: Datong Sun <[email protected]>
dndx
changed the title
dndx
changed the title
This interface allows the caller to validate an expression string with schema and get its operators/fields without needing a Router instance.
KAG-5013