fix: client.add not adding buffers properly

[JohnTJohnston]

Fixed while getting AD thumbnailPhoto to work.

To repro, add a thumbnailPhoto to AD.

Note: I don't believe I ever reported this problem, but I've been using the fix for a long time.

[UziTech]

Can you add a test?

[JohnTJohnston]

Sorry, I must have missed something in your documentation. I don't know what is assumed by your test runs. I need to be able to add a thumbnail photo to AD. Do you run your tests against AD? Can I count on the existence of a particular user?

[jsumners]

To repro, add a thumbnailPhoto to AD.

Sorry. This is not an effective means of proving the change. First, not everyone has access to an Active Directory instance. Second, it does not prevent regressions in future releases. A unit test is required to satisfy these requirements.

[JohnTJohnston]

Sorry, I'm not clear what is expected of your unit tests. How are they supposed to work without a back end directory to verify functionality? Not sure what other attributes require passing a buffer into add, so how to test?

Seems like I may have to live with my private patches...

[jsumners]

Sorry, I'm not clear what is expected of your unit tests. How are they supposed to work without a back end directory to verify functionality? Not sure what other attributes require passing a buffer into add, so how to test?

Seems like I may have to live with my private patches...

The client unit tests are in https://github.com/ldapjs/node-ldapjs/blob/91de94d1a625b66b753091697632cbc6f433913d/test/client.test.js

We further have "integration" tests at https://github.com/ldapjs/node-ldapjs/tree/91de94d1a625b66b753091697632cbc6f433913d/test-integration/client

The integration tests use an standard OpenLDAP instance as built by https://github.com/ldapjs/docker-test-openldap and provided via https://github.com/ldapjs/docker-test-openldap/pkgs/container/docker-test-openldap/openldap

The integration tests are useful in cases where writing a unit test is either too difficult or not sufficient.

Note: I need to do some work to update the Docker repository to GitHub's latest, truly anonymous, access mechanisms. But it is attainable currently through authenticated means.

[JohnTJohnston]

I am not familiar with openldap attributes. this bug fix is specfic to attributes that require a javascript buffer to be passed in, and for all I know, openldap might have been smart enough not to have such attributes.

[jsumners]

I am not familiar with openldap attributes. this bug fix is specfic to attributes that require a javascript buffer to be passed in, and for all I know, openldap might have been smart enough not to have such attributes.

We really can't merge patches without a test to cover their functionality. This PR addresses reading of binary attributes, yes? My initial theory would be that updating the server embedded in the unit tests to return an entry that has a Buffer valued attribute would allow for a unit test to be written to cover this change.

[JohnTJohnston]

This was necessary to modify a binary attribute. This following test passes, but it passes both before and after the fix.

tap.test('add success with buffer', function (t) {
  const entry = {
    someAttribute: Buffer.alloc(20)
  }
  t.context.client.add('cn=add, ' + SUFFIX, entry, function (err, res) {
    t.error(err)
    t.ok(res)
    t.equal(res.status, 0)
    t.end()
  })
})

[jsumners]

This following test passes, but it passes both before and after the fix.

Then we should investigate why.

[jsumners]

Awesome! This is what we need to help improve this library. The test fails before the patch and succeeds after.

[jsumners]

⚠️ This issue has been locked due to age. If you have encountered a recent
problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example
when opening a new issue.