add basic ldif reader #49

vetinari · 2016-02-14T11:14:17Z

No description provided.

liggitt · 2016-02-18T04:22:22Z

ldif.go

+// A basic LDIF parser. This one does currently just support LDIFs like
+// they're generated by tools like ldapsearch(1) / slapcat(8). Change
+// records are not supported.
+type LDIF struct {


mixing the parser and the data is unusual... I would expect an LDIFParser that takes a reader and decodes into an LDIF struct

or maybe something along the lines of https://golang.org/pkg/encoding/json/#Unmarshal, e.g.

ldif.Unmarshal(io.Reader, *[]ldap.Entry) error

if you wanted the relaxed parser option, that could be in a decoder modeled after https://golang.org/pkg/encoding/json/#Decoder, e.g.

var r io.Reader = ... var e *[]ldap.Entry = ... decoder := ldif.NewDecoder(r) decoder.RelaxedParsing(true) err := decoder.Decode(e)

liggitt · 2016-02-18T05:47:29Z

thanks for the PR!

vetinari · 2016-02-18T19:30:29Z

I'll fix the issues, this may take some time

- move to own package - fix some parser errors (comment continuation, attr:val w/o spaces) - extend the LDIF struct to support change records in the future

vetinari · 2016-03-20T09:44:59Z

pending changes for Marshal() depend on #54

enochtsang · 2016-08-04T18:59:27Z

Hi I've actually been using this branch even though is hasn't been merged yet, I've noticed that entries must be separated by exactly one line, no more. I'm not sure if this strict whitespacing is intended but the ldapadd utility is not as strict.

# Organization Units
dn: ou=users,dc=example,dc=com
objectClass: organizationalUnit
objectClass: top
ou: users


# searches for above empty line for dn but fails and errors out in this PR
# Even though this is a valid LDIF file for ldapadd
dn: ou=groups,dc=example,dc=com
objectClass: organizationalUnit
objectClass: top
ou: groups

I would like to comment that it seems like as well as Modify not yet being supported as in the comments, Add and Delete are also not being parsed for yet as far as I can tell. It's not a huge deal feature wise but the comments should probably add that Add and Delete are also not supported yet (if I'm not mistaken haha).

EDIT: I see that @vetinari is still adding features, ignore the above paragraph.

liggitt · 2016-08-04T19:20:52Z

ldif/ldif.go

+		return nil, errors.New("empty entry?")
+	}
+
+	if l.Version == 0 && strings.HasPrefix(lines[0], "version:") {


seems like it would be better to make the version parsing an optional thing done on the first line of the file, rather than part of parseEntry. as it is, the first entry could be missing version line, and the second entry have one, and this would parse it out as the ldif version

fixed now in 8232180

vetinari · 2016-08-05T06:17:36Z

@johnweldon now there's nothing left... well, except for the mod(r)dn changes, but those depend on #54

vetinari · 2016-08-05T06:21:27Z

@enochtsang it's not explicitly stated that Add / Delete and Modify entries are not supported, but the head of ldif/ldif.go states that

This one currently just supports LDIFs like they are generated by tools like ldapsearch(1) slapcat(8). Change records are not supported while unmarshalling.

enochtsang · 2016-08-05T15:12:20Z

ldif/ldif_test.go

+
+
+# searches for above empty line for dn but fails and errors out in this PR
+# Even though this is a valid LDIF file for ldapadd


This looked a little familiar haha, these two comments aren't necessary anymore I don't think. Thanks for the fix!

enochtsang · 2016-09-12T15:28:12Z

Can this pull request be merged?

vetinari · 2016-09-13T09:58:41Z

from my POV, yes.

johnweldon · 2016-09-13T16:23:35Z

ldif/apply.go

+		case entry.Add != nil:
+			if err := conn.Add(entry.Add); err != nil {
+				if continueOnErr {
+					log.Printf("ERROR: Failed to add %s: %s", entry.Add.DN, err)


Maybe as a future TODO, can we use an io.Writer here, or maybe pass in the log rather than using the static global?

johnweldon · 2016-09-13T16:35:41Z

Other than the ModifyDN commented code (can we remove that or is there a compelling reason to leave that commented code in?), and a couple minor issues, this seems mergeable to me.

After addressing the trivial items and maybe filing an issue for the io.Writer or passed in log to fix in the future, I'm fine with merging this. @liggitt any objections?

liggitt · 2016-09-13T16:58:22Z

I don't object, though I haven't played with the API enough to know if there are improvements we'll want. That makes tagging versions tricky, if there is new, experimentalish code side-by-side with code we want to stay version compatible with.

liggitt · 2016-09-13T17:00:10Z

I've been thinking about starting a v2 maintenance branch, and working in master to resolve some of the issues tagged for v3 that involve API changes or dropping support for old go versions. How would you feel about this just living in master until that work was done and we reached a v3.0.0 tag?

vetinari · 2016-09-13T17:14:34Z

After addressing the above issues, what about putting the ldif/ dir as a new repo (https://github.com/go-ldap/ldif) and then maybe make it accessible as gopkg.in/ldif.v0 / v1?

liggitt · 2016-09-13T17:17:02Z

I like the ability to version the ldif stuff separately, and starting with v0 seems like a good way to clarify the expectations around API stability until we've had more time with it

johnweldon · 2016-09-13T17:17:38Z

That sounds good to me too - @liggitt do you want to make the new repo, or should I?

liggitt · 2016-09-13T18:15:48Z

created at https://github.com/go-ldap/ldif

vetinari · 2016-09-18T13:55:14Z

PR done: go-ldap/ldif#3

liggitt · 2016-09-21T19:37:00Z

thanks, merged

add basic ldif reader

626a90b

liggitt reviewed Feb 18, 2016
View reviewed changes

vetinari added 10 commits February 19, 2016 20:17

Rework

5b2dc4d

- move to own package - fix some parser errors (comment continuation, attr:val w/o spaces) - extend the LDIF struct to support change records in the future

moved

33174f5

marshal ldif.LDIF{}s

47139c1

FoldWidth for marshalling

553040a

export struct values

9b058ac

add marshalling of change requests

039842e

allow no line folding with FoldWidth < 0

b28402d

doc update

f5b1f54

fixes, add EntriesAsLDIF()/ChangesAsLDIF()

80a4fdf

tests for marshalling changes + fixes for those

88b0c4c

vetinari mentioned this pull request Apr 20, 2016

Export AddRequest Struct variables in add.go #60

Merged

liggitt reviewed Aug 4, 2016
View reviewed changes

vetinari added 2 commits August 5, 2016 07:55

fix error where multiple blank lines cause panic while parsing

94b60a2

enforce optional "version: " only on first entry

8232180

enochtsang reviewed Aug 5, 2016
View reviewed changes

Merge branch 'master' into ldif-reader

80c0aa1

johnweldon reviewed Sep 13, 2016
View reviewed changes

vetinari mentioned this pull request Sep 18, 2016

import go-ldap/ldif#3

Merged

liggitt closed this Sep 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add basic ldif reader #49

add basic ldif reader #49

vetinari commented Feb 14, 2016

liggitt Feb 18, 2016

liggitt Feb 18, 2016

liggitt commented Feb 18, 2016

vetinari commented Feb 18, 2016

vetinari commented Mar 20, 2016

enochtsang commented Aug 4, 2016 •

edited

Loading

liggitt Aug 4, 2016 •

edited

Loading

vetinari Aug 5, 2016

vetinari commented Aug 5, 2016

vetinari commented Aug 5, 2016

enochtsang Aug 5, 2016 •

edited

Loading

enochtsang commented Sep 12, 2016

vetinari commented Sep 13, 2016

johnweldon Sep 13, 2016

johnweldon commented Sep 13, 2016

liggitt commented Sep 13, 2016

liggitt commented Sep 13, 2016

vetinari commented Sep 13, 2016

liggitt commented Sep 13, 2016

johnweldon commented Sep 13, 2016

liggitt commented Sep 13, 2016

vetinari commented Sep 18, 2016

liggitt commented Sep 21, 2016



		# searches for above empty line for dn but fails and errors out in this PR
		# Even though this is a valid LDIF file for ldapadd

add basic ldif reader #49

add basic ldif reader #49

Conversation

vetinari commented Feb 14, 2016

liggitt Feb 18, 2016

Choose a reason for hiding this comment

liggitt Feb 18, 2016

Choose a reason for hiding this comment

liggitt commented Feb 18, 2016

vetinari commented Feb 18, 2016

vetinari commented Mar 20, 2016

enochtsang commented Aug 4, 2016 • edited Loading

liggitt Aug 4, 2016 • edited Loading

Choose a reason for hiding this comment

vetinari Aug 5, 2016

Choose a reason for hiding this comment

vetinari commented Aug 5, 2016

vetinari commented Aug 5, 2016

enochtsang Aug 5, 2016 • edited Loading

Choose a reason for hiding this comment

enochtsang commented Sep 12, 2016

vetinari commented Sep 13, 2016

johnweldon Sep 13, 2016

Choose a reason for hiding this comment

johnweldon commented Sep 13, 2016

liggitt commented Sep 13, 2016

liggitt commented Sep 13, 2016

vetinari commented Sep 13, 2016

liggitt commented Sep 13, 2016

johnweldon commented Sep 13, 2016

liggitt commented Sep 13, 2016

vetinari commented Sep 18, 2016

liggitt commented Sep 21, 2016

enochtsang commented Aug 4, 2016 •

edited

Loading

liggitt Aug 4, 2016 •

edited

Loading

enochtsang Aug 5, 2016 •

edited

Loading