Skip to content

Conversation

@letterbeezps
Copy link
Contributor

No description provided.

json.go Outdated
}

return ioutil.WriteFile(exportPath, jsonString, os.ModePerm)
if _, err = f.WriteString("]"); err != nil {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When possible, like in this case, I prefer using the := operator

err = db.ForEach(q, func(doc *d.Document) bool {
n++
jsonByte, err := json.Marshal(doc.AsMap())
if err != nil {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a good idea to recover this error in the outer scope.
If there is any failure inside this function, then the export process would not report an error, but the result would be clearly not correct.

json.go Outdated
if n != collectionCount {
jsonString += ","
}
_, err = f.WriteString(jsonString)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess same thing for this error

Copy link
Owner

@ostafen ostafen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, @letterbeezps, first of all thank you for your contribution, you are doing a great job with the last PRs. The structure looks good, I only left some minor comments.

@ostafen ostafen linked an issue Jun 21, 2023 that may be closed by this pull request

jsonString, err := json.Marshal(docs)
n := 0
err = db.ForEach(q, func(doc *d.Document) bool {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Count() and the ForEach() statements run in separate transactions. Thus, if new documents are inserted between them, the ForEach() statement will iterate a number of documents n > collectionCount, which would lead to some problem with the statement:

if n != collectionCount {
	jsonString += ","
}

To prevent this issue, we can ensure the number of documents iterated by the ForEach() is exactly collectionCount using Limit():

err = db.ForEach(q.Limit(collectionCount), func(doc *d.Document) bool {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You remind me that is can also cause problems when documents are deleted between them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a suggestion for ForEach() function, let consumer() return error instead of bool. If there is any failure inside this function, we can get the error info directly

func (db *DB) ForEach(q *query.Query, consumer func(_ *d.Document) error) error {
	q, err := normalizeCriteria(q)
	if err != nil {
		return err
	}

	return db.IterateDocs(q, func(doc *d.Document) error {
		if err := consumer(doc); err != nil {
			return err
		}
		return nil
	})
}

json.go Outdated

result, err := db.FindAll(query.NewQuery(collectionName))
q := query.NewQuery(collectionName)
collectionCount, err := db.Count(q)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just call this variable count, for simplicity

@letterbeezps letterbeezps requested a review from ostafen June 21, 2023 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory efficient ExportCollection()

2 participants