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

[API Proposal]: Create an AsnReader from an AsnReader #86723

Closed
vcsjones opened this issue May 24, 2023 · 6 comments · Fixed by #86913
Closed

[API Proposal]: Create an AsnReader from an AsnReader #86723

vcsjones opened this issue May 24, 2023 · 6 comments · Fixed by #86913
Labels
api-approved API was approved in API review, it can be implemented area-System.Formats.Asn1
Milestone

Comments

@vcsjones
Copy link
Member

Background and motivation

The AsnReader is great for forward-only reading, but sometimes you want to use it, but not consume it; or you want to be able to put it back to how it was before you read through it.

It would be nice if there was a way to copy a reader with its current state. The Peek methods don't work particularly well for this scenario, since that only gives you the data for the next encoded value, not any trailing data.

I would propose a constructor on AsnReader that takes an AsnReader, and duplicates the state.

API Proposal

namespace System.Formats.Asn1;

public partial class AsnReader
{
    public AsnReader(AsnReader reader);
}

API Usage

public void ProcessData(AsnReader reader) {
    AsnReader clone = new AsnReader(reader);

    while (clone.HasData) {
        ReadOnlyMemory<byte> value = clone.ReadEncodedValue();
        // Do something with the value
    }

    // The reader parameter has not been advanced
}

Alternative Designs

This could be a Copy or Clone instance method, but my preference leans toward a new constructor.

Risks

No response

@vcsjones vcsjones added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Formats.Asn1 labels May 24, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 24, 2023
@ghost
Copy link

ghost commented May 24, 2023

Tagging subscribers to this area: @dotnet/area-system-formats-asn1, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

The AsnReader is great for forward-only reading, but sometimes you want to use it, but not consume it; or you want to be able to put it back to how it was before you read through it.

It would be nice if there was a way to copy a reader with its current state. The Peek methods don't work particularly well for this scenario, since that only gives you the data for the next encoded value, not any trailing data.

I would propose a constructor on AsnReader that takes an AsnReader, and duplicates the state.

API Proposal

namespace System.Formats.Asn1;

public partial class AsnReader
{
    public AsnReader(AsnReader reader);
}

API Usage

public void ProcessData(AsnReader reader) {
    AsnReader clone = new AsnReader(reader);

    while (clone.HasData) {
        ReadOnlyMemory<byte> value = clone.ReadEncodedValue();
        // Do something with the value
    }

    // The reader parameter has not been advanced
}

Alternative Designs

This could be a Copy or Clone instance method, but my preference leans toward a new constructor.

Risks

No response

Author: vcsjones
Assignees: -
Labels:

api-suggestion, area-System.Formats.Asn1

Milestone: -

@bartonjs
Copy link
Member

Seems reasonable to me. I've probably done it with the internal AsnValueReader by just saving a copy. Structs are nice for copying (but terrible for side-effecting) :)

@bartonjs bartonjs added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels May 24, 2023
@bartonjs bartonjs added this to the Future milestone May 24, 2023
@adamsitnik
Copy link
Member

Creating a copy of a class to have a state that we can go back to seems a little bit unintuitive for me. As Jeremy wrote, we often do it for structs, but for reference types I would expect to see sth like:

public class AsnReader
{
    public int Position { get; set; }
}

But this would require non trivial changes to the type itself (it's based on slicing as far as I can see).

So the proposal LGTM ;)

@vcsjones
Copy link
Member Author

vcsjones commented May 26, 2023

public int Position { get; set; }

One reason I like the current design, and not a position, is a Position would allow you to drop in to arbitrary locations = and since a SEQUENCE is also a an AsnReader, you might end up doing a lot of book keeping on the position.

Consider this ASN.1, in BER: 3005090380000A0101010101

This is a SEQUENCE that contains a REAL with trailing data.

If I start reading it at Position 1, now all of the sudden it becomes an ASN.1 NULL (a weird one).

The current design really doesn't let you do that, which I think is a good characteristic.

@adamsitnik
Copy link
Member

The current design really doesn't let you do that, which I think is a good characteristic.

👍

@terrajobst
Copy link
Member

terrajobst commented May 30, 2023

Video

  • Seems like a typed Clone() is easier to use
namespace System.Formats.Asn1;

public partial class AsnReader
{
    public AsnReader Clone();
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 30, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 30, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 2, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Formats.Asn1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants