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.

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

Isolate scoped settings in default OData services #3071

Merged
merged 5 commits into from
Sep 25, 2024
Merged

Isolate scoped settings in default OData services #3071

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
35a19c5
Add tests
habbes Sep 23, 2024
f8ce020
Clone scoped prototypes
habbes Sep 23, 2024
c18b4b9
Minor refactor
habbes Sep 23, 2024
dd5687f
Refactor clone
habbes Sep 23, 2024
1aaddbc
Refactor clone
habbes Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Microsoft.OData.Core/ODataServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,15 @@ public static IServiceCollection AddDefaultODataServices(this IServiceCollection
// Finally, register configurable settings.
var readerSettings = new ODataMessageReaderSettings(odataVersion);
configureReaderAction?.Invoke(readerSettings);
services.AddScoped(sp => readerSettings);
services.AddScoped(sp => readerSettings.Clone());
Copy link
Member

Choose a reason for hiding this comment

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

question: Would there be an issue if we called configure reader action multiple times e.g. if we used a delegate instead of the cloned instance?

Copy link
Contributor Author

@habbes habbes Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Are you referring to this approach:

services.AddScope(_ => {
    var readerSettings = new ODataMessageReaderSettings(...);
    configureReaderAction?.Invoke(readerSettings);
    return readerSettings;
});

If so, I considered it (and mentioned it in the attached issue). The reason I opted for cloning instead of that approach is because we can't predict the cost of the delegate since it's user defined, for all we know it could be reading from a file or database. On the other hand, since we control the implementation of the Clone() method, we can make better assumptions about it cost and we can optimize if it becomes a hotspot, so I think it's safer to err on the side of calling Clone() multiple times rather than calling the opaque user-defined delegate multiple times.


var writerSettings = new ODataMessageWriterSettings(odataVersion);
configureWriterAction?.Invoke(writerSettings);
services.AddScoped(sp => writerSettings);
services.AddScoped(sp => writerSettings.Clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

When an ODataMessageWriterSettings is passed as argument to an ODataMessageWriter, the scoped ODataMessageWriterSettings service is being updated to that value?

Copy link
Contributor Author

@habbes habbes Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@syprieur That's a very good question, apparently it does :( The settings from the ODataMessageWriterSettings are copied to the scoped ODataMessageWriterSettings service:

see:

which calls:

internal static ODataMessageWriterSettings CreateWriterSettings(
    IServiceProvider container,
    ODataMessageWriterSettings other)
{
    ODataMessageWriterSettings writerSettings;
    if (container == null)
    {
        writerSettings = new ODataMessageWriterSettings();
    }
    else
    {
       // we get the scoped settings
        writerSettings = container.GetRequiredService<ODataMessageWriterSettings>();
    }

    if (other != null)
    {
        // then overwrite the scoped service based on the settings passed to the writer
        writerSettings.CopyFrom(other);
    }

    return writerSettings;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

That explains the observed bug and confirms the hypothesis: the same settings instance is indeed re-used and updated concurrently.
Your change will fix it.


var parserSettings = new ODataUriParserSettings();
configureUriParserAction?.Invoke(parserSettings);
services.AddScoped(sp => parserSettings);
services.AddScoped(sp => parserSettings.Clone());

return services;
}
Expand Down
13 changes: 13 additions & 0 deletions src/Microsoft.OData.Core/UriParser/ODataUriParserSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,19 @@ public ODataUriParserSettings()
this.EnableParsingKeyAsSegment = true;
}

internal ODataUriParserSettings Clone()
{
ODataUriParserSettings copy = new ODataUriParserSettings();
copy.FilterLimit = this.FilterLimit;
copy.OrderByLimit = this.OrderByLimit;
copy.PathLimit = this.PathLimit;
copy.SearchLimit = this.SearchLimit;
copy.MaximumExpansionDepth = this.MaximumExpansionDepth;
copy.MaximumExpansionCount = this.MaximumExpansionCount;
copy.EnableParsingKeyAsSegment = this.EnableParsingKeyAsSegment;
return copy;
habbes marked this conversation as resolved.
Show resolved Hide resolved
}

/// <summary>
/// Gets or sets the maximum depth of the tree that results from parsing $expand.
/// </summary>
Expand Down
71 changes: 70 additions & 1 deletion test/FunctionalTests/Microsoft.OData.Core.Tests/ODataServiceCollectionExtensionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
// </copyright>
//---------------------------------------------------------------------

using System;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.OData.Edm;
using Microsoft.OData.Json;
using Microsoft.OData.UriParser;
using System;
using Xunit;

namespace Microsoft.OData.Tests
Expand Down Expand Up @@ -136,6 +136,29 @@ public void AddDefaultODataServices_ReaderSettings_CanConfigure()
Assert.True(readerSettings.EnableCharactersCheck);
}

[Fact]
public void AddDefaultODataServices_ReaderSettings_InstancesAreScoped()
{
var services = new ServiceCollection();
services.AddDefaultODataServices();
var provider = services.BuildServiceProvider();
using var scope1 = provider.CreateScope();
var settings = scope1.ServiceProvider.GetRequiredService<ODataMessageReaderSettings>();
settings.EnableCharactersCheck = true;

var settingsFromSameScope = scope1.ServiceProvider.GetRequiredService<ODataMessageReaderSettings>();

using var scope2 = provider.CreateScope();
var settingsFromOtherScope = scope2.ServiceProvider.GetRequiredService<ODataMessageReaderSettings>();

// Instances from the same scope should be the same
Assert.True(object.ReferenceEquals(settings, settingsFromSameScope));
Assert.True(settingsFromSameScope.EnableCharactersCheck);
// Instances from different scopes should be different
Assert.False(object.ReferenceEquals(settings, settingsFromOtherScope));
Assert.False(settingsFromOtherScope.EnableCharactersCheck);
}

/// <summary>
/// Tests whether the <see cref="ODataMessageReaderSettings" /> can be configured using the <see cref="Action{ODataMessageWriterSettings}" />.
/// </summary>
Expand All @@ -160,6 +183,29 @@ public void AddDefaultODataServices_WriterSettings_CanConfigure()
Assert.True(writerSettings.EnableCharactersCheck);
}

[Fact]
public void AddDefaultODataServices_WriterSettings_InstancesAreScoped()
{
var services = new ServiceCollection();
services.AddDefaultODataServices();
var provider = services.BuildServiceProvider();
using var scope1 = provider.CreateScope();
var settings = scope1.ServiceProvider.GetRequiredService<ODataMessageWriterSettings>();
settings.EnableCharactersCheck = true;

var settingsFromSameScope = scope1.ServiceProvider.GetRequiredService<ODataMessageWriterSettings>();

using var scope2 = provider.CreateScope();
var settingsFromOtherScope = scope2.ServiceProvider.GetRequiredService<ODataMessageWriterSettings>();

// Instances from the same scope should be the same
Assert.True(object.ReferenceEquals(settings, settingsFromSameScope));
Assert.True(settingsFromSameScope.EnableCharactersCheck);
// Instances from different scopes should be different
Assert.False(object.ReferenceEquals(settings, settingsFromOtherScope));
Assert.False(settingsFromOtherScope.EnableCharactersCheck);
}

/// <summary>
/// Tests whether the <see cref="ODataUriParserSettings" /> can be configured using the <see cref="Action{ODataUriParserSettings}" />.
/// </summary>
Expand All @@ -184,6 +230,29 @@ public void AddDefaultODataServices_ODataUriParserSettings_CanConfigure()
Assert.Equal(1, parserSettings.MaximumExpansionCount);
}

[Fact]
public void AddDefaultODataServices_ODataUriParserSettings_InstancesAreScoped()
{
var services = new ServiceCollection();
services.AddDefaultODataServices();
var provider = services.BuildServiceProvider();
using var scope1 = provider.CreateScope();
var settings = scope1.ServiceProvider.GetRequiredService<ODataUriParserSettings>();
settings.EnableParsingKeyAsSegment = false;

var settingsFromSameScope = scope1.ServiceProvider.GetRequiredService<ODataUriParserSettings>();

using var scope2 = provider.CreateScope();
var settingsFromOtherScope = scope2.ServiceProvider.GetRequiredService<ODataUriParserSettings>();

// Instances from the same scope should be the same
Assert.True(object.ReferenceEquals(settings, settingsFromSameScope));
Assert.False(settingsFromSameScope.EnableParsingKeyAsSegment);
// Instances from different scopes should be different
Assert.False(object.ReferenceEquals(settings, settingsFromOtherScope));
Assert.True(settingsFromOtherScope.EnableParsingKeyAsSegment);
}

/// <summary>
/// Tests whether the correct exception is thrown when no <see cref="IServiceCollection" /> is provided.
/// </summary>
Expand Down
Loading