CSHARP-3355: Add more spec tests for invalid relaxed UUIDs. #452

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

Jump to bottom

Merged

DmitryLukyanov merged 1 commit into mongodb:master from DmitryLukyanov:csharp3355

Feb 18, 2021

Merged

CSHARP-3355: Add more spec tests for invalid relaxed UUIDs. #452

DmitryLukyanov merged 1 commit into mongodb:master from DmitryLukyanov:csharp3355

Feb 18, 2021

Conversation

@DmitryLukyanov

Copy link

Contributor

@DmitryLukyanov DmitryLukyanov commented Feb 5, 2021

EG: https://evergreen.mongodb.com/version/601d956b3e8e86586c978855

@DmitryLukyanov DmitryLukyanov requested a review from jyemin

February 5, 2021 18:59

jyemin

jyemin requested changes

Feb 5, 2021

View reviewed changes

tests/MongoDB.Bson.Tests/Specifications/bson-corpus/tests/bsonview Show resolved Hide resolved

@jyemin jyemin requested a review from JamesKovacs

February 5, 2021 19:03

@DmitryLukyanov DmitryLukyanov requested a review from jyemin

February 5, 2021 19:14

rstam

rstam requested changes

Feb 5, 2021

View reviewed changes

src/MongoDB.Bson/IO/JsonReader.cs Outdated

{

throw new FormatException($"Invalid $uuid hyphens format: '{uuidTokenStringValue}'.");

}

Copy link

Contributor

@rstam rstam Feb 5, 2021

Choose a reason for hiding this comment

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

I think you can replace lines 2093-2112 with:

var guid = Guid.Parse(uuidToken.StringValue);
var binaryData = new BsonBinaryData(guid, GuidRepresentation.Standard);

Guid.Parse already does all the validations we need.

Copy link

Contributor Author

@DmitryLukyanov DmitryLukyanov Feb 5, 2021

Choose a reason for hiding this comment

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

good point, done

@DmitryLukyanov

Copy link

Contributor Author

DmitryLukyanov commented Feb 5, 2021

EG: https://evergreen.mongodb.com/version/601d9af00305b9115d9a8acd

@DmitryLukyanov DmitryLukyanov requested a review from rstam

February 5, 2021 19:23

@jyemin jyemin removed the request for review from JamesKovacs

February 5, 2021 19:43

@jyemin

Copy link

Contributor

jyemin commented Feb 5, 2021

Removed James since Robert picked it up.

rstam

rstam approved these changes

Feb 11, 2021

View reviewed changes

Copy link

Contributor

@rstam rstam left a comment

Choose a reason for hiding this comment

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

LGTM

JamesKovacs

JamesKovacs requested changes

Feb 13, 2021

View reviewed changes

Copy link

Contributor

@JamesKovacs JamesKovacs left a comment

Choose a reason for hiding this comment

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

Changes look good. Verified that:

BsonBinarySubType is still set correctly.
Same exception type (FormatException) is thrown as before.
Tests run successfully.

One minor change required. You have an extraneous using statement.

src/MongoDB.Bson/IO/JsonReader.cs Outdated

using System.Globalization;

using System.IO;

using System.Text.RegularExpressions;

using System.Linq;

Copy link

Contributor

@JamesKovacs JamesKovacs Feb 13, 2021

Choose a reason for hiding this comment

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

Not used.

Copy link

Contributor Author

@DmitryLukyanov DmitryLukyanov Feb 13, 2021

Choose a reason for hiding this comment

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

Done

@DmitryLukyanov DmitryLukyanov requested a review from JamesKovacs

February 13, 2021 00:29

JamesKovacs

JamesKovacs approved these changes

Feb 17, 2021

View reviewed changes

Copy link

Contributor

@JamesKovacs JamesKovacs left a comment

Choose a reason for hiding this comment

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

LGTM

@DmitryLukyanov DmitryLukyanov force-pushed the csharp3355 branch from cb5295b to 636be2f Compare

February 18, 2021 15:20

@DmitryLukyanov


 CSHARP-3355: Add more spec tests for invalid relaxed UUIDs.

2c0e72b

@DmitryLukyanov DmitryLukyanov force-pushed the csharp3355 branch from 636be2f to 2c0e72b Compare

February 18, 2021 15:21

@DmitryLukyanov DmitryLukyanov merged commit 2c0e72b into mongodb:master

Feb 18, 2021

@DmitryLukyanov DmitryLukyanov deleted the csharp3355 branch

February 18, 2021 15:23

Labels

None yet

4 participants

@DmitryLukyanov @jyemin @JamesKovacs @rstam

CSHARP-3355: Add more spec tests for invalid relaxed UUIDs. #452

CSHARP-3355: Add more spec tests for invalid relaxed UUIDs. #452

Uh oh!

Conversation

@DmitryLukyanov DmitryLukyanov commented Feb 5, 2021

Uh oh!

Uh oh!

@rstam rstam Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

@DmitryLukyanov DmitryLukyanov Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

DmitryLukyanov commented Feb 5, 2021

Uh oh!

jyemin commented Feb 5, 2021

Uh oh!

@rstam rstam left a comment

Choose a reason for hiding this comment

Uh oh!

@JamesKovacs JamesKovacs left a comment

Choose a reason for hiding this comment

Uh oh!

@JamesKovacs JamesKovacs Feb 13, 2021

Choose a reason for hiding this comment

Uh oh!

@DmitryLukyanov DmitryLukyanov Feb 13, 2021

Choose a reason for hiding this comment

Uh oh!

@JamesKovacs JamesKovacs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants