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

System.Resources API additions for non-primitive objects #29324

Closed
ericstj opened this issue Apr 19, 2019 · 3 comments
Closed

System.Resources API additions for non-primitive objects #29324

ericstj opened this issue Apr 19, 2019 · 3 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Resources
Milestone

Comments

@ericstj
Copy link
Member

ericstj commented Apr 19, 2019

Goals

Enable compilation of WindowsForms applications that use non-primitive resources on .NETCore. Avoid creating live objects when building resources for non-primitive types. Set a path moving forward to avoid using BinaryFormatter.

This can be done by passing through pre-serialized data from resx to resources file.
image

Draft Implementation : dotnet/corefx#36906

Naming pivots

Assembly / Namespace : System.Resources.Extensions

  • options: 1 vs 2 assemblies? I've tried both. 1 is easier for maintenance and testing. Dependencies of the reader are the same as writer. Size of writer is 23KB, reader is 46KB.
  • naming:
    • System.ComponentModel.Resources
    • System.Resources.Serialization
    • System.Resources.PassThrough
    • System.Resources.Extensions
    • System.Resources.Data
    • System.Resources.Binary

Types:

  • Nouns: ResourceReader, ResourceSet, and ResourceWriter
  • Prefixes:
    • Serializing
    • PassThrough
    • Data
    • Object
    • Component
    • Converting
    • Preserialized
    • Deserializing

Proposed API

// optionally sealed if we don't want extensibility
public partial class DeserializingResourceReader : System.Resources.IResourceReader
{
    // all of these are present on ResourceReader
    public DeserializingResourceReader(System.IO.Stream stream) { }
    public DeserializingResourceReader(string fileName) { }
    public void Close() { }
    public void Dispose() { }
    public System.Collections.IDictionaryEnumerator GetEnumerator() { throw null; }
    public void GetResourceData(string resourceName, out string resourceType, out byte[] resourceData) { throw null; }
    System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; }

    // Optional additions if we want to support extensibility
    protected virtual int Version { get { throw null; } }
    protected virtual object DeserializeObject(System.IO.BinaryReader reader, Type type) { throw null; }
}

// optionally sealed if we don't want extensibility
public partial class PreserializedResourceWriter : System.Resources.IResourceWriter
{
    // all of these are present on ResourceWriter
    public PreserializedResourceWriter(System.IO.Stream stream) { }
    public PreserializedResourceWriter(string fileName) { }
    public System.Func<System.Type, string> TypeNameConverter { get { throw null; } set { } }
    public void AddResource(string name, byte[] value) { }
    public void AddResource(string name, System.IO.Stream value) { }
    public void AddResource(string name, System.IO.Stream value, bool closeAfterWrite) { }
    public void AddResource(string name, object value) { }
    public void AddResource(string name, string value) { }
    // existing pass through member that doesn't encode any deserialization format
    public void AddResourceData(string name, string typeName, byte[] serializedData) { }
    public void Close() { }
    public void Dispose() { }
    public void Generate() { }

    // Optional additions if we want to support extensibility
    protected virtual string ResourceReaderTypeName { get { throw null; } }
    protected virtual string ResourceSetTypeName { get { throw null; } }
    protected void AddResourceData(string name, string typeName, object dataContext) { }
    protected virtual void WriteData(System.IO.BinaryWriter writer, object dataContext) { }

    // Below are new methods that behave similarly to AddResourceData but imply a different form 
    // of pre-serialized data.
    
    // Similar to AddResourceData but encodes deserialization format, necessary to distinguish
    // between pure pass-through case. 
    public void AddBinaryFormattedResourceData(string name, string typeName, byte[] value) { }

    // Data is wrapped in a stream and passed to Activator.CreateInstance
    // This is what gets used by ResXFileRef in resx.
    public void AddStreamResourceData(string name, string typeName, byte[] value) { }
    public void AddStreamResourceData(string name, string typeName, System.IO.Stream value, bool closeAfterWrite) { }

    // Data is passed to typeconverter as either byte[] or string respectively
    public void AddTypeConverterResourceData(string name, string typeName, byte[] value) { }
    public void AddTypeConverterResourceData(string name, string typeName, string value) { }	
}

// not shown internal class RuntimeResourceSet 

Alternatives

expand

  1. Extend ResourceReader/Writer

    1. Types are frozen in existing frameworks - won't work for MSBuild which needs to run on desktop.
    2. Doesn't solve the problem for existing frameworks, which MSBuild needs to support
    3. See details
  2. Build into ResourceReader/Writer

    1. Essentially imagine the API in this proposal on the existing types instead of new types.
    2. Same issues as option 1.
    3. Requires a revision to resources format or some convention for defining serialization methods.
  3. Wrap ResourceReader/Writer and .resources format

    1. Requires same API as this proposal, but doesn't copy as much code.
    2. Implementation & payload size will be less efficient due to wrapping
  4. Extend only ResourceSet

    1. Still requires Writer API, though one could imagine building this only using ResourceWriter.AddResourceData if you were able to change the ResourceSet name in the generated resources or get the designer to pass it in.
    2. Without a matching Reader is much less efficient than RuntimeResourceSet due to lookup methods being internal. Once you add a ResourceReader it's effectively the same as the proposal, but places deserialization in ResourceSet instead of the Reader.
    3. This really exposes a gap in the IResourceReader / ResourceReader abstraction. If lookup strategy must be implemented for reasonable perf, then we should update the abstractions. One could consider this as our opportunity to do that, using the unsealed virtual Reader as the new abstraction.

Details

If we change inbox types and can constrain runtime to only .NETCoreApp 3.0 we would do the following.
dotnet/corefx@bbb08de#diff-9a2d6c26f251ad72e355afb8bcb72ca2L18

// previously sealed
public class ResourceWriter : System.Resources.IResourceWriter
{
    // new 
    protected virtual string ResourceReaderTypeName { get { throw null; } }
    protected virtual string ResourceSetTypeName { get { throw null; } }
    protected void AddResourceData(string name, string typeName, object dataContext) { }
    protected virtual void WriteData(System.IO.BinaryWriter writer, object dataContext) { }
}

dotnet/corefx@bbb08de#diff-147dad3b8d1fabfe7d53faa577442011L69

// previously sealed
public class ResourceReader : System.Resources.IResourceReader
{
    // new 
    protected virtual int Version { get { throw null; } }
    protected virtual object DeserializeObject(System.IO.BinaryReader reader, Type type) { throw null; }
}

In addition to these API additions, we need to make RuntimeResourceSet usable for derived ResourceReaders by adding a IResourceReader constructor that can upcast to a ResourceReader to give lookup-behavior. For the current scenarios we don't need it to be made public / unsealed, but it could be.

internal class RuntimeResourceSet
{
    // new 
   internal RuntimeResourceSet(IResourceReader reader) { } // if reader is ResourceReader, will do lazy loading.
}

@ericstj ericstj changed the title [Draft] System.Resources API additions for non-primitive objects System.Resources API additions for non-primitive objects Apr 22, 2019
@ericstj ericstj self-assigned this Apr 22, 2019
@terrajobst
Copy link
Member

terrajobst commented Apr 23, 2019

Video

Looks good:

  • Seal the types and make the protected members private or remove them
  • closeAfterWrite should be defaulted
  • Remove the TypeNameConverter, assuming MSBuild doens't need it

@ericstj
Copy link
Member Author

ericstj commented Apr 23, 2019

TypeNameConverter isn't used by MSBuild, the only case this was used on ResourceWriter in the framework was for multitargeting by System.Web. Since we aren't looking to plumb this writer to System.Web we can leave that out.

GetResourceData on the reader is and AddResourceData on the writer both provide raw access to the binary data on representing the types. We can decide not to expose these as we don't have a scenario for it. They were used originally by loc tooling that operated on .resources rather than source resx. Let's leave these off until we have a scenario for it.

We also mentioned that AddStreamResourceData can be changed to AddActivatorResourceData and only take a stream (so that the method signature indicates the primitive type passed to Activator, similar to what we're doing with AddTypeConverterResourceData).

@tarekgh
Copy link
Member

tarekgh commented Apr 26, 2019

Closing this as I think @ericstj already merged his changes dotnet/corefx#36906

@tarekgh tarekgh closed this as completed Apr 26, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2020
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.Resources
Projects
None yet
Development

No branches or pull requests

4 participants