Skip to content

Implement IEnumerable<> on various X509 collection types #53129

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.

Sign up for GitHub

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
bartonjs merged 6 commits into from
Jun 4, 2021
Merged

Implement IEnumerable<> on various X509 collection types #53129

bartonjs merged 6 commits into from
Jun 4, 2021

Conversation

@hrrrrustic
Copy link
Contributor

@hrrrrustic hrrrrustic commented May 22, 2021

close #49878

@ghost
Copy link

ghost commented May 22, 2021

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented May 22, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

close #49878

Author: hrrrrustic
Assignees: -
Labels:

area-System.Security, new-api-needs-documentation
Milestone: -

@stephentoub
Copy link
Member

stephentoub commented May 23, 2021

Thanks. Note the linked issue linked to a commit that sketched out the changes (stephentoub@d310e84), and as part of that, it updated places that were then unnecessarily using OfType... it'd be good to do the same here. Also, this PR should also be adding tests exercising the new interfaces (or if existing tests end up exercising the new interfaces, that then means they're no longer exercising the old ones, so new tests should be added for those).

@hrrrrustic hrrrrustic marked this pull request as draft May 25, 2021 12:50
@hrrrrustic
Copy link
Contributor Author

hrrrrustic commented May 31, 2021

I'm a little bit confused with adding tests

All existed tests seems like that

runtime/src/libraries/System.Security.Cryptography.X509Certificates/tests/CollectionTests.cs

Lines 174 to 203 in 3846a9c

private static void TestNonGenericEnumerator(IEnumerator e, object c1, object c2, object c3)
{
object ignored;
for (int i = 0; i < 2; i++)
{
// Not started
Assert.Throws<InvalidOperationException>(() => ignored = e.Current);
Assert.True(e.MoveNext());
Assert.Same(c1, e.Current);
Assert.True(e.MoveNext());
Assert.Same(c2, e.Current);
Assert.True(e.MoveNext());
Assert.Same(c3, e.Current);
Assert.False(e.MoveNext());
Assert.False(e.MoveNext());
Assert.False(e.MoveNext());
Assert.False(e.MoveNext());
Assert.False(e.MoveNext());
// Ended
Assert.Throws<InvalidOperationException>(() => ignored = e.Current);
e.Reset();
}
}

or that

runtime/src/libraries/System.Security.Cryptography.X509Certificates/tests/CollectionTests.cs

Lines 380 to 395 in 3846a9c

public static void X509CertificateCollectionEnumeratorModification()
{
using (X509Certificate c1 = new X509Certificate())
using (X509Certificate c2 = new X509Certificate())
using (X509Certificate c3 = new X509Certificate())
{
X509CertificateCollection cc = new X509CertificateCollection(new X509Certificate[] { c1, c2, c3 });
X509CertificateCollection.X509CertificateEnumerator e = cc.GetEnumerator();
cc.Add(c1);
// Collection changed.
Assert.Throws<InvalidOperationException>(() => e.MoveNext());
Assert.Throws<InvalidOperationException>(() => e.Reset());
}
}

Should I really add exactly same for typed IEnumerable/IEnumerator?

@bartonjs
Copy link
Member

bartonjs commented Jun 1, 2021

Should I really add exactly same for typed IEnumerable/IEnumerator?

Since it's the same method bodies providing the behavior, nah.

@hrrrrustic hrrrrustic marked this pull request as ready for review June 2, 2021 09:13
@bartonjs bartonjs requested a review from stephentoub June 3, 2021 22:28
bartonjs
bartonjs reviewed Jun 3, 2021
View reviewed changes
Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

LGTM. Added Steve to see if he has any remaining thoughts based on the prototype/investigation work.

stephentoub
stephentoub reviewed Jun 4, 2021
View reviewed changes
....Security.Cryptography.X509Certificates/ref/System.Security.Cryptography.X509Certificates.cs
public void Reset() { }
bool System.Collections.IEnumerator.MoveNext() { throw null; }
void System.Collections.IEnumerator.Reset() { }
void System.IDisposable.Dispose() { }
Copy link
Member

@stephentoub stephentoub Jun 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@bartonjs, opinion on this being explicitly implemented vs implicitly? (here and for the other dispose methods)

Copy link
Member

@bartonjs bartonjs Jun 4, 2021

Choose a reason for hiding this comment

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

For a sealed type that most people never use directly where it's a no-op merely required by interface inheritance? Nah.

stephentoub
stephentoub approved these changes Jun 4, 2021
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks!

@bartonjs bartonjs merged commit 7f5265a into dotnet:main Jun 4, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 4, 2021
@bartonjs bartonjs added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@vcsjones vcsjones vcsjones left review comments

@stephentoub stephentoub stephentoub approved these changes

@bartonjs bartonjs bartonjs approved these changes

Assignees

No one assigned

Labels

area-System.Security needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration new-api-needs-documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

API Proposal: Implement IEnumerable<> on various X509 collection types

4 participants

@hrrrrustic @stephentoub @bartonjs @vcsjones