Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOCSP-23996 Add enableUtf8Validation Connectionstring Option #576

Merged
merged 38 commits into from
Jan 19, 2024

Conversation

ianf-mongodb
Copy link
Contributor

@ianf-mongodb ianf-mongodb commented Nov 6, 2023

DESCRIPTION

This edit adds mention to the nodejs / Compass URI option enableUtf8Validation which allows Compass to show collections that contain invalid UTF8 data. This additional option needs mention on https://www.mongodb.com/docs/drivers/node/current/fundamentals/connection/connection-options/, it works as both a client parameter and a URI connection string option.

Parsing logic: https://github.com/mongodb/js-bson/blob/main/src/parser/deserializer.ts#L678.

STAGING

https://preview-mongodbianfmongodb.gatsbyjs.io/compass/DOCSP-23996/query/filter/#query-collections-with-invalid-utf8-data

JIRA

https://jira.mongodb.org/browse/DOCSP-23996

BUILD LOG

https://workerpool-boxgs.mongodbstitch.com/pages/job.html?collName=queue&jobId=654bfb1f5b686d6bd982a45d

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Is this free of spelling errors?
  • Is this free of grammatical errors?
  • Is this free of staging / rendering issues?
  • Are all the links working?

External Review Requirements

What's expected of an external reviewer?

@ianf-mongodb
Copy link
Contributor Author

@sarah-olson-mongodb Could you take a look at this whenever you get a minute please?

Copy link
Collaborator

@sarah-olson-mongodb sarah-olson-mongodb left a comment

Choose a reason for hiding this comment

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

Thanks @ianf-mongodb. This is a lot of questions / comments. Sorry about that. Let's for sure talk this over tomorrow!

source/includes/fact-non-utf8-data.rst Outdated Show resolved Hide resolved
source/includes/fact-non-utf8-data.rst Outdated Show resolved Hide resolved
Comment on lines 3 to 4
can disable UTF8 validation by setting the ``enableUtf8Validation``
URI option to ``false``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

(question/context)
What are the ramifications of disabling UTF-8 validation. Do we "recommend" that you do this in all/some situations?

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 added a warning callout, that this should be a temporarily method.

source/includes/fact-non-utf8-data.rst Show resolved Hide resolved
Copy link
Contributor Author

@ianf-mongodb ianf-mongodb left a comment

Choose a reason for hiding this comment

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

Hi @sarah-olson-mongodb this is ready for another look.

source/includes/fact-non-utf8-data.rst Show resolved Hide resolved
source/includes/fact-non-utf8-data.rst Outdated Show resolved Hide resolved
source/includes/fact-non-utf8-data.rst Outdated Show resolved Hide resolved
Comment on lines 3 to 4
can disable UTF8 validation by setting the ``enableUtf8Validation``
URI option to ``false``.
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 added a warning callout, that this should be a temporarily method.

Copy link
Collaborator

@sarah-olson-mongodb sarah-olson-mongodb left a comment

Choose a reason for hiding this comment

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

Hey @ianf-mongodb, sorry about the long delay and thanks so much for these updates! Here you go! A bunch of minor stuff here and one request for further detail. Happy to chat as needed. Otherwise, let me know when it's ready for another quick look.

Comment on lines 1 to 5
Compass can have issues displaying collections if documents have
invalid UTF8 characters.

If you attempt to query or export this data, the following error
message displays:
Copy link
Collaborator

Choose a reason for hiding this comment

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

(minimalism -- suggestion)
What if you combined the first two sentences. Something like:
"If you attempt to query or export data with invalid UTF8 characters ..."

Invalid UTF-8 string in BSON document.

In order to query or export this data, you can disable
UTF8 validation by setting the ``enableUtf8Validation`` URI option to
Copy link
Collaborator

Choose a reason for hiding this comment

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

(minimalism)
Nice work getting Vale integrated with the Compass repo.
Adding to the Vale suggestion, consider something like:
"To query or export this data, disable UTF8 ...

.. warning::

**Editing data** with ``enableUtf8Validation=false`` can result in
potential loss of data. This approach is a temporary workaround to
Copy link
Collaborator

Choose a reason for hiding this comment

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

(suggestion)
consider "can result in data loss."
Reasoning is that "can" and "potential" are a bit redundant in this sentence.

source/includes/fact-non-utf8-data.rst Show resolved Hide resolved
source/includes/fact-non-utf8-data.rst Outdated Show resolved Hide resolved
source/includes/fact-non-utf8-data.rst Outdated Show resolved Hide resolved
@ianf-mongodb
Copy link
Contributor Author

@sarah-olson-mongodb Ready for re-review

Copy link
Collaborator

@sarah-olson-mongodb sarah-olson-mongodb left a comment

Choose a reason for hiding this comment

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

Thanks a ton for your work on this @ianf-mongodb. LGTM!

@ianf-mongodb
Copy link
Contributor Author

@lerouxb This is ready for review

@mdb-ashley mdb-ashley merged commit e8eaace into mongodb:master Jan 19, 2024
1 check passed
@docs-builder-bot
Copy link

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.

5 participants