So, in this case (aka Python 2 branch) you return empty string? Not good. Either make it work or fail utterly.

You can ''.join([('\\%02x' ..... and not have to bother with magic on the return line below then, or even better -- just return here.

Superfluous line with a trailing space.

Trailing space.

My impression is this should only be used for search filters (not DN encoding). If that's the case, please name it more precisely, something like search_filter_scape_bytes -- I know it's longer but easier to understand what it actually does on first sight.

Missing empty line at the end of the file.

Finally, please create a unit test for this function. Make sure to also include boundary values, like:
- empty input
- single zero byte
- zero byte in between non-zero bytes
- general case (e.g. first few (ten-ish) bytes of a DER certificate)
Too long values are unnecessary to test in this case.

Thanks.

So, in this case (aka Python 2 branch) you return empty string? Not good. Either make it work or fail utterly.

Done

You can ''.join([('\%02x' ..... and not have to bother with magic on the return line below then, or even better -- just return here.

Done

Superfluous line with a trailing space.

Removed

Trailing space.

Removed

My impression is this should only be used for search filters (not DN encoding). If that's the case, please name it more precisely, something like search_filter_scape_bytes -- I know it's longer but easier to understand what it actually does on first sight.

corrected

Missing empty line at the end of the file.

corrected

Finally, please create a unit test for this function. Make sure to also include boundary values, like:
- empty input
- single zero byte
- zero byte in between non-zero bytes
- general case (e.g. first few (ten-ish) bytes of a DER certificate)
Too long values are unnecessary to test in this case.
Thanks.

This is its implementation https://pagure.io/389-ds-base/pull-request/50434

Finally, please create a unit test for this function. Make sure to also include boundary values, like:
- empty input
- single zero byte
- zero byte in between non-zero bytes
- general case (e.g. first few (ten-ish) bytes of a DER certificate)
Too long values are unnecessary to test in this case.
Thanks.

I am creating some test cases now .

Finally, please create a unit test for this function. Make sure to also include boundary values, like:
- empty input
- single zero byte
- zero byte in between non-zero bytes
- general case (e.g. first few (ten-ish) bytes of a DER certificate)
Too long values are unnecessary to test in this case.
Thanks.

Now i have added some test cases in src/lib389/lib389/tests/utils_test.py as you have suggested

Why not follow the logic in the origin function?
with:

if bytes_value:
    # do conversion
else:
    # set an empty string to the result

this way we'll avoid the unnecessary exception processing.

Why not follow the logic in the origin function?
with:
if bytes_value:
# do conversion
else:
# set an empty string to the result

this way we'll avoid the unnecessary exception processing.

This was done as per @mhonek 's suggestions .

So, in this case (aka Python 2 branch) you return empty string? Not good. Either make it work or fail utterly.

Why not follow the logic in the origin function?
with:
if bytes_value:
# do conversion
else:
# set an empty string to the result

this way we'll avoid the unnecessary exception processing.

Isn't throwing an exception and then properly handling it on a higher level better than returning a semantically incorrect value and then have to spend ages finding out where the hell the empty string somewhere in the search filter (once you recognized the fact the value in the middle of the string is actually missing) came from? (btw, the incorrectly typed function's argument should be an assert failure in the first place) Or do I miss something? I believe it is better to tell the end-user we failed than pretend we did the job correctly although we surely didn't in this case.

By a zero byte what I actually meant is a byte with value zero, like ASCII NUL, 0x00 in hexadecimal format. That to be sure we don't mess up just because of some implementation specifics (I know, this is not C, but still).

Why not follow the logic in the origin function?
with:
if bytes_value:
# do conversion
else:
# set an empty string to the result
this way we'll avoid the unnecessary exception processing.

Isn't throwing an exception and then properly handling it on a higher level better than returning a semantically incorrect value and then have to spend ages finding out where the hell the empty string somewhere in the search filter (once you recognized the fact the value in the middle of the string is actually missing) came from? (btw, the incorrectly typed function's argument should be an assert failure in the first place) Or do I miss something? I believe it is better to tell the end-user we failed than pretend we did the job correctly although we surely didn't in this case.

I had/have the impression that this function is more like ensure function. If it encounters an empty value - it just passes it through.
As you said in the comment, we probably should check for an empty input in other places.

I am ok-ish (depends on the context we want to use the function in, but then we should include a maybe (or similar) in its name) with passing through (i.e. not modifying) null values (e.g. None, b'', '', [], (), etc.). However we cannot allow returning an empty string on e.g. ['Hello', 1] which is what it was doing before IIUC.

The original function from ldap3 seems to handle the null values by returning an empty string -- is it good? Maybe, depending on what we agree the use-cases to be.

By a zero byte what I actually meant is a byte with value zero, like ASCII NUL, 0x00 in hexadecimal format. That to be sure we don't mess up just because of some implementation specifics (I know, this is not C, but still).

Corrected , please check

I am ok-ish (depends on the context we want to use the function in, but then we should include a maybe (or similar) in its name) with passing through (i.e. not modifying) null values (e.g. None, b'', '', [], (), etc.). However we cannot allow returning an empty string on e.g. ['Hello', 1] which is what it was doing before IIUC.
The original function from ldap3 seems to handle the null values by returning an empty string -- is it good? Maybe, depending on what we agree the use-cases to be.

I think it may give flexibility. Like we can pass an empty list and it will give an empty search filter.
Like, when we write any search filter - it is always str. So it makes sense to encapsulate the conversion to the required type in the ensure function.
But we should name the function it properly then and write the docstring.

Also, I agree that it really depends on the code style and coding approach.
I'll leave it to you and @aborah to decide because it is done for your tests now.

I am okay with both options but this part should be documented in the docstring for both cases, I think.

@aborah We had a look with Simon and we agreed on an approach. Please, if you agree (see also the commit message in it), apply the patch from here, and if it passes the test please rebase and squash it. Thanks.

@aborah We had a look with Simon and we agreed on an approach. Please, if you agree (see also the commit message in it), apply the patch from here, and if it passes the test please rebase and squash it. Thanks.

Done

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3794

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.