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

RUBY-1768 change to_s to return original URI #2596

Merged
merged 9 commits into from
Oct 12, 2022

Conversation

Neilshweky
Copy link
Contributor

@Neilshweky Neilshweky commented Aug 12, 2022

No description provided.

@p-mongo
Copy link
Contributor

p-mongo commented Aug 12, 2022

I think a bigger question here is what will happen if the original URI was partially invalid and the invalid parts were dropped during parsing. to_s would now return something that is technically an invalid URI right?

If you do decide to return @string I think you would need to dup it otherwise clients can end up mutating it.

@p-mongo
Copy link
Contributor

p-mongo commented Aug 12, 2022

Also what happens for SRV URIs?

@Neilshweky
Copy link
Contributor Author

@p-mongo that's a good point, I learned that options with invalid values should be dropped (when srvMaxHosts is negative, for example), so I decided to reconstruct the uri from its parts. I have added tests for regular URIs and srv URIs.

Copy link
Contributor

@p-mongo p-mongo left a comment

Choose a reason for hiding this comment

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

What do you think about having ability to retrieve both the original uri and the "correct" uri? And designated methods for this, e.g. to_uri and original_uri. The "URI" in class name makes this confusing because technically URI is the string, a URI parsed into components is strictly no longer a URI.

end
end

context 'read preference max staleness option provided' do

let(:options) do
'readPreference=Secondary&maxStalenessSeconds=120'
'readPreference=secondary&maxStalenessSeconds=120'
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this lowercasing behavior have a test?

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'm going to put this back as well

spec/mongo/uri_spec.rb Outdated Show resolved Hide resolved
lib/mongo/uri.rb Outdated
end

private

# Reconstruct the URI from its parts.
Copy link
Contributor

Choose a reason for hiding this comment

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

So this should say that invalid option values are dropped and option names are converted to lower|canonical case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Neilshweky Neilshweky force-pushed the RUBY-1768 branch 2 times, most recently from 3f671f9 to bc5c991 Compare September 1, 2022 22:10
@Neilshweky
Copy link
Contributor Author

@p-mongo this is rfal

@Neilshweky Neilshweky removed the request for review from p-mongo September 29, 2022 17:08
lib/mongo/uri.rb Outdated Show resolved Hide resolved
@Neilshweky Neilshweky merged commit 829b7fb into mongodb:master Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants