ldapjs / node-ldapjs

LDAP Client and Server API for node.js
http://ldapjs.org
MIT License
1.61k stars 448 forks source link

entry received on a searchEntry message does not contain object #841

Closed VirgileD closed 1 year ago

VirgileD commented 1 year ago

Hello, I'm updating a project and switch to 3.0.0. I had to change the code (obviously) from

client.search('cn=groups,dc=rd,dc=test,dc=company,dc=fr', { filter: `(&(memberuid=USERID`, scope: "sub", attributes: "cn"}, function(err, res) {
   res.on('searchEntry', function(entry) {
   groups.push(entry.object.cn)
}});

to

client.search('cn=groups,dc=rd,dc=test,dc=company,dc=fr', { filter: `(&(memberuid=USERID`, scope: "sub", attributes: "cn"}, function(err, res) {
   res.on('searchEntry', function(entry) {
   groups = groups.concat(entry.pojo.attributes.filter(f => f.type=="cn").map(m => m.values)[0])
}});

Obviously the "object" field of the returned entry is gone and I didn't find anything else but the "pojo" field to retrieve the results of the search. Not a big problem though but it took me some time to figure it out and the documentation should be changed accordingly unless I missed something.

jsumners commented 1 year ago

Yep. That's a change in the way the message objects are constructed and accessed. If you want a "plain JavaScript" object of a message then .pojo is the accessor you're looking for.

However, in this specific case, I would skip .pojo and do:

groups = groups.concat(
  entry.attributes.filter(f => f.type === 'cn').map(m -> m.values).pop()
)

Thank you for highlighting this change. I have updated the release notes to reflect this item https://github.com/ldapjs/node-ldapjs/releases/tag/v3.0.0.

VirgileD commented 1 year ago

ah, ok, I had missed the entry.attributes part, makes sense. Thank you!

jsumners commented 1 year ago

Understandable. We need some prominent docs around everything. Unfortunately, the jsdoc gets lost in the spaghetti that is the client/server code. It would be more evident when using the libraries directly. Hopefully this all gets sorted out in time, but will be a bit difficult for quite some time.

mil7 commented 1 year ago

Hi there, is there any plan to provide a more convenient access to the entry's attributes?

I am curious, what is the reason for this change? We heavily depended on the v2's object representation of the attributes.

The most striking difference: Earlier we (as client) received two kinds different kinds of values:

We just trusted this incoming representation. Now we have to know in our client which attribute's value is of array and which of string because we have to distinguish the handling of an array of length 1.

Maybe we did not understand LDAP well enough.

jsumners commented 1 year ago

is there any plan to provide a more convenient access to the entry's attributes?

What is more convenient than entry.attributes?

I am curious, what is the reason for this change?

It's in the release notes: an instance of SearchResultEntry is an object.

Maybe we did not understand LDAP well enough.

https://www.rfc-editor.org/rfc/rfc4511.html#section-4.5.2 defines the spec for SearchResultEntry. The attributes property is clearly defined as an array of attributes.

The .object property in the v2 SearchEntry object did the following:

https://github.com/ldapjs/node-ldapjs/blob/92dfc80fd169f889cf2e0de53ff140ef2887c1c7/lib/messages/search_entry.js#L40-L63

It's not documented as to why that property even exists, nor why it would return a completely different representation than the spec defined object. The SearchResultEntry in @ldapjs/messages strictly follows the spec, and provides a .pojo ("plain old JavaScript object") accessor to generate a stripped down object that can be rendered as JSON or supplied as an "options" object to new SearchResultEntry(<object>) (a design decision for every message supported in @ldapjs/messages).

DeanDavis commented 1 year ago

is there any plan to provide a more convenient access to the entry's attributes?

What is more convenient than entry.attributes?

Pre change if I did a search with...

var opts = { //options for client.search
filter: TAFilter,
scope: 'sub',
attributes: ['mail', 'thumbnailPhoto', 'memberOf', 'displayName', 'sAMAccountName', 'l']
}

I could access the attributes with... entry.object["mail"] entry.object.memberOf etc.

Now my understanding is the syntax is entry.attributes.filter(f => f.type === 'mail').map(m => m.values).pop()); So, yes the old way was more convenient and understandable. I'm sure more than one way to do this but all of them reduce to the earlier simplicity Is this breaking change a pain, yes, but in the grand scheme of things not the worst thing in the world.

jsumners commented 1 year ago

PRs to improve the SearchResultEntry with convenience accessors are welcome.

foxxyz commented 1 year ago

I'm a little saddened that RFC specs are referenced as a good reason to abandon a human-friendly API.

I get it, specs are important and it's important to follow them, but I'd argue generally the objective of libraries like this should be to provide an abstraction of a complex or archaic system into a simpler model. To that end, the previous way to access attributes was highly superior to the current version.

For anyone who'd like to get similar structure to the previous .object accessor, I've been using this:

const attributes = Object.fromEntries(entry.attributes.map(({ type, values }) => [type, values.length > 1 ? values : values[0]]))

Just to be clear, I'm grateful for this library, and thank you for maintaining it. Just don't forget there are humans on the other end.