-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[dotnet] Implement third-party Permissions module #16414
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
b595dfc
eaa3d5c
71a7489
1fbf4c6
9a26d1e
dbe8e0b
6449944
43dd406
341338f
8d3b1b0
6bec772
875f5e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,29 +18,28 @@ | |
// </copyright> | ||
|
||
using OpenQA.Selenium.BiDi.Communication; | ||
using OpenQA.Selenium.BiDi.Communication.Json; | ||
using System.Text.Json; | ||
using System.Text.Json.Serialization; | ||
|
||
namespace OpenQA.Selenium.BiDi; | ||
|
||
public abstract class Module | ||
{ | ||
protected Broker Broker { get; private set; } | ||
|
||
internal BiDiJsonSerializerContext JsonContext { get; private set; } | ||
internal JsonSerializerContext JsonContext { get; private set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not ready to generalize it in this PR. Still not clear how to move on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have an example in this PR’s description, of how to move forward with these changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I though making There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Observed it is theoretical changes. Let be practicable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is, except for the shared JSON context introduced in #16402 It is not theoretical changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I came to the following:
Then changes looks simple. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel introducing new
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All right, |
||
|
||
protected virtual void Initialize(JsonSerializerOptions options) { } | ||
protected abstract JsonSerializerContext CreateJsonContext(JsonSerializerOptions options); | ||
|
||
internal static TModule Create<TModule>(BiDi bidi, Broker broker, JsonSerializerOptions jsonOptions, BiDiJsonSerializerContext context) | ||
public static TModule Create<TModule>(BiDi bidi, JsonSerializerOptions jsonOptions, JsonSerializerContext? cachedContext = null) | ||
where TModule : Module, new() | ||
{ | ||
TModule module = new() | ||
{ | ||
Broker = broker, | ||
JsonContext = context | ||
Broker = bidi.Broker, | ||
}; | ||
|
||
module.Initialize(jsonOptions); | ||
module.JsonContext = cachedContext ?? module.CreateJsonContext(jsonOptions); | ||
|
||
return module; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// <copyright file="PermissionDescriptor.cs" company="Selenium Committers"> | ||
// Licensed to the Software Freedom Conservancy (SFC) under one | ||
// or more contributor license agreements. See the NOTICE file | ||
// distributed with this work for additional information | ||
// regarding copyright ownership. The SFC licenses this file | ||
// to you under the Apache License, Version 2.0 (the | ||
// "License"); you may not use this file except in compliance | ||
// with the License. You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, | ||
// software distributed under the License is distributed on an | ||
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
// KIND, either express or implied. See the License for the | ||
// specific language governing permissions and limitations | ||
// under the License. | ||
// </copyright> | ||
|
||
namespace OpenQA.Selenium.BiDi.Permissions; | ||
|
||
internal record PermissionDescriptor(string Name); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// <copyright file="PermissionState.cs" company="Selenium Committers"> | ||
// Licensed to the Software Freedom Conservancy (SFC) under one | ||
// or more contributor license agreements. See the NOTICE file | ||
// distributed with this work for additional information | ||
// regarding copyright ownership. The SFC licenses this file | ||
// to you under the Apache License, Version 2.0 (the | ||
// "License"); you may not use this file except in compliance | ||
// with the License. You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, | ||
// software distributed under the License is distributed on an | ||
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
// KIND, either express or implied. See the License for the | ||
// specific language governing permissions and limitations | ||
// under the License. | ||
// </copyright> | ||
|
||
using OpenQA.Selenium.BiDi.Communication.Json.Converters; | ||
using System.Text.Json.Serialization; | ||
|
||
namespace OpenQA.Selenium.BiDi.Permissions; | ||
|
||
[JsonConverter(typeof(CamelCaseEnumConverter<PermissionState>))] | ||
public enum PermissionState | ||
{ | ||
Granted, | ||
Denied, | ||
Prompt | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
// <copyright file="PermissionsExtensions.cs" company="Selenium Committers"> | ||
// Licensed to the Software Freedom Conservancy (SFC) under one | ||
// or more contributor license agreements. See the NOTICE file | ||
// distributed with this work for additional information | ||
// regarding copyright ownership. The SFC licenses this file | ||
// to you under the Apache License, Version 2.0 (the | ||
// "License"); you may not use this file except in compliance | ||
// with the License. You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, | ||
// software distributed under the License is distributed on an | ||
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
// KIND, either express or implied. See the License for the | ||
// specific language governing permissions and limitations | ||
// under the License. | ||
// </copyright> | ||
|
||
using OpenQA.Selenium.BiDi.Extensions.Permissions; | ||
using System; | ||
|
||
namespace OpenQA.Selenium.BiDi.Permissions; | ||
|
||
public static class PermissionsExtensions | ||
{ | ||
public static PermissionsModule AsPermissions(this BiDi bidi) | ||
{ | ||
if (bidi is null) | ||
{ | ||
throw new ArgumentNullException(nameof(bidi)); | ||
} | ||
|
||
return Module.Create<PermissionsModule>(bidi, bidi.DefaultBiDiOptions()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RenderMichael this piece is strange, seems there is no reason to keep JsonContext in base class. If you have the same opinion, I can remake it to private field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base class is responsible for creating the JSON context. This is the only way to bridge the gap between "strongly-typed JSON context" and the logic in Module.Create
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to get around this is by making the Json Context a generic parameter, eg.
BrowserModule : Module<BiDiJsonSerializerContext>
. That requires making BiDiJsonSerializerContext public, and more publicly exposed. I don't think that's desirable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also was thinking about
BrowserModule : Module<BiDiJsonSerializerContext>
, but it seems even stranger. Module of JsonContext... I am reading it hard, given thatBiDiJsonSerializerContext
cannot be inherited.Ideally we should get rid of
BiDiJsonSerializerContext
, we are moving in this direction.About
internal new BiDiJsonSerializerContext JsonContext => (BiDiJsonSerializerContext)base.JsonContext;
: For me it is risky, at least not obvious...void Initialize(...)
or thisimplicit initialization
JsonContext
getter?Private field looks really simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to go that route, then we will have to undo optimizations such as this one: #16397
Otherwise, we will need a separate JSON context for each module
What do you have in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second optimization that would need to be de-optimized: #16392
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internal modules will still use global JsonContext for a while, because of f**g cross-referencing (stateful converters, polymorphic dto). In future, each module wants to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we do cannot have a global JsonContext for third-party extensions, it looks like we will need 2 different kinds of modules: extensions and non-extension (core?).
That shouldn't be a problem! We can get rid of stateful converters (very slightly less pleasant usability, I think it is no problem) and polymorphic DTOs got addressed by moving the converters out to attributes.
The only remaining converter is
DateTimeOffsetConverter
. We could go through to each instance ofDateTimeOffset
and add that converter as an attribute. Alternatively, we could change the type tolong
in the DTO, since it is technically magic. If we want to address that, then we will have no converters left!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DateTimeOffsetConverter
is not a problem, it is kind of common JsonOptions. it is OK to have global well-known converters.I don't want to block this PR, we can improve later. But seems
private MyJsonContext
should be here.Broker
requiresJsonTypeInfo
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each module should have own JsonContext, ideally. And it is out of scope of this PR.