Add a way to overwrite the OAuth parameters on challenge requests (e.g. "scope", or the OIDC "prompt")

This is a follow-up to #1401 which introduced the ability to specify the prompt for OpenID Connect authentication requests to the authentication provider. As per the spec, this will control how the authentication provider will behave.
In the original pull request, this was added as a global option for the authentication scheme. However, there are use cases where specifying the prompt on specific requests would be very useful. In particular, the prompt=login value will request a “reauthentication”, making this a useful thing to make the user confirm that they can actually log in with the authentication provider; but having this as a global option would be too much.
This pull requests adds the ability to configure the prompt value using the AuthenticationProperties object that is passed to ChallengeAsync. An example use could look like this:
var props = new AuthenticationProperties();
props.Items.Add(OpenIdConnectDefaults.PromptPropertiesKey, "login");
await HttpContext.ChallengeAsync(OpenIdConnectDefaults.AuthenticationScheme, props);

I also included a test for this change, and a test for the original change that introduced the prompt configuration.

@Tratcher - can you log a bug to track this item?

https://github.com/aspnet/Security/issues/1698

Can we also adopt the same pattern in the OAuth2 generic provider? There's a recurrent demand for supporting it in the aspnet-contrib providers.

@PinpointTownes that's what https://github.com/aspnet/Security/issues/990 proposes. Feel free to discuss there.

Is there a particular reason to have different tickets for what is essentially the same feature (with the same design and the same implementation)? :sweat_smile:

Because this PR only covers one of them 😁

I’d be happy to expand this PR to also cover OAuth if that won’t break the scope for 2.1 (i.e. I would really like this to be shipped with 2.1).
While the implementation within the handlers would be different, I think we could still share some common code for this (e.g. for retrieving the arguments from the AuthenticationProperties).
For OAuth, there would be the same question though: What parameters do we want to be able to overwrite? The authentication requests don’t actually have that many arguments, and as per above, I wouldn’t want things like response_type, client_id or redirect_uri overwritable. So for bare OAuth2, that would only leave scope. But looking at the Google example, it might make sense to allow for users to add additional query arguments; ones that are not covered directly by the spec but might trigger some special behavior on the authentication provider.
My biggest issue right now would be the discoverability of this on properties. Right now, taking the Google provider as an example, you actually need to know (a) that you can overwrite query arguments, (b) which you can overwrite (e.g. you cannot currently add new ones), and (c) how to actually add those and what key name to use.
What do you think about having extension methods on th e AuthenticationProperties object to overwrite query arguments? Something like this:
// OAuth
AuthenticationProperties OverwriteScope(params string[] scopes);

// OIDC
AuthenticationProperties OverwritePrompt(string prompt);
AuthenticationProperties OverwriteMaxAge(TimeSpan maxAge);

// Google
AuthenticationProperties OverwriteGoogleLoginHint(string loginHint);
AuthenticationProperties OverwriteGoogleAccessType(string accessType);
AuthenticationProperties OverwriteIncludeGrantedScopes(bool includeGrantedScopes);

// and maybe even
AuthenticationProperties AddAuthenticationQueryArgument(string key, string value);

This would solve the problem mentioned above that we would have to validate the items. By having properly typed extension methods, we could just assume that the type is correct on the item and wouldn’t have to have an explicit validation there.
Since this would build on top of the existing solution with the Google provider (for backwards compatibility), this would also solve my other question from above: The key names within the Items collection would match the exact query argument name. So for OAuth, one could use the OpenIdConnectParameterNames. But with extension methods, of course one would not have to remember that exactly.
What do you think? Would that be too much to be included for 2.1? In that case, we could also split this up, so we can ship this just for OIDC for now and then expand to the rest later. We would do it in a forwards compatible way, so we wouldn’t introduce problems later.
I’ll be gone for the weekend, so I cannot work on this until Monday, but I’ll gladly come up with a first draft for this then, after deciding in what direction we want to head here.

So for bare OAuth2, that would only leave scope.

Yep. That's what folks want to customize in most cases.

As for scheduling, the work you've completed can readily go into preview2. If it were expanded it would likely slip to RC, but the longer it takes the less likely it will be accepted for 2.1.

Okay, then I'll make sure to have something ready on Monday so that this won't be delayed.
What do you think about my proposed solution with extension methods though? Would that be an acceptable approach?

The extension method approach is interesting though you may have trouble making them generic enough. Otherwise I worry you'd need a lot of them and they'd get noisy.
FYI: https://github.com/aspnet/Security/blob/d24d4fa99f46822a3b9b87702507e0dc914a0126/src/Microsoft.AspNetCore.Authentication.Facebook/FacebookHandler.cs#L70-L73
See what you can get done in a day or two. Skip OAuth if you need to, we can come back to that. I'd want to check each of the providers to see what makes sense to add/override.

My 2-cent about this change:

Only standard parameters that have a high chance of being customized per-challenge should be supported. scope, prompt, login_hint (and maybe max_age) are the best candidates.

I don't think it's worth supporting adding other parameters. It's already something you can already do anyway (using the events model):

services.AddAuthentication()
    .AddOpenIdConnect(options =>
    {
        options.Events.OnRedirectToIdentityProvider = context =>
        {
            // Replace the scope generated by the OIDC handler.
            if (context.Properties.Items.TryGetValue("scope", out var scope))
            {
                context.ProtocolMessage.Scope = scope;
            }

            // Add a custom parameter.
            if (context.Properties.Items.TryGetValue("custom_parameter", out var parameter))
            {
                context.ProtocolMessage.SetParameter("custom_parameter", parameter);
            }

            return Task.CompletedTask;
        };
    });

@poke and I chatted about the extensions on Slack and I think this would add too much noise. On the other hand, having a bunch of (relatively easy to discover) "properties" keys in the const classes would be nice.

Okay, I’ve pushed some more commits to address everything. I will do a clean rebase when the review is done.
After discussing with @PinpointTownes, I decided against the extension methods as we would really end up with too many different extensions (especially when some providers are doing things differently). Instead, each {Provider}Defaults class has a set of constants that will show which parameters can be overwritten. And users are simply expected to provide valid values there (we cannot really perform a useful validation on the values there).
This also goes partly in the direction of #990 but I didn’t completely replace the custom code in the GoogleHandler yet.

Updated the PR again with OAuthChallengeProperties objects. While this means four new types, I don’t think it’s an “explosion of sub types” and I actually like how usable the code becomes:
context.ChallengeAsync("Google", new GoogleChallengeProperties
{
    Scope = "https://www.googleapis.com/auth/plus.login",
    AccessType = "offline",
    ApprovalPrompt = "force",
    Prompt = "consent",
    LoginHint =<EMAIL_ADDRESS>    IncludeGrantedScopes = false,
});

I think that looks really nice. What do you think?

Its too bad we can't make it nice with lambdas easily since we only have a single Challenge method... oh well... regardless this looks much better to me as well!

The explosion I'm concerned about is over here: https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/tree/dev/src

@Tratcher that's indeed a valid (and annoying) concern, but if @HaoK can't live with the constant approach, not sure we have many options.
@HaoK would introducing an indexer make the syntax more tolerable for you?
await context.ChallengeAsync("Google", new AuthenticationProperties
{
    [OAuthConstants.Properties.Scope] = "openid profile email"
});

FWIW, we're already planning to introduce *Constants classes for all the aspnet-contrib providers to expose the custom claims we support. We could certainly add a Properties class to expose these properties as well without creating more than 40 *ChallengeProperties classes :sweat_smile:

The explosion I'm concerned about is over here: https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/tree/dev/src

So I just went through all the contrib handlers and it turns out that there are actually very few cases, where a customization (over the default OAuthChallengeProperties object) would be necessary:

AspNet.Security.OAuth.Weixin – Uses a comma for scope separation like Facebook, so would need its own WeixinChallengeProperties object (without additional properties).
AspNet.Security.OAuth.Reddit – Currently has a duration=permanent challenge query argument. We could make this customizable which would mean a RedditChallengeProperties object, but the only other value is duration=temporary which means that the token expires after an hour. I am not convinced that this is very useful.

In addition, there are a few options that are not query arguments during the challenge request but actually are parameters that are used in the UserInformationEndpoint request when the ticket is created:

AspNet.Security.OAuth.Reddit – Has a UserAgent option which could theoretically be configured on a per challenge basis. But if we were to do this, this would require its completely own logic (since it’s not there for the challenge redirect, but instead for all backchannel requests). So this would be out of scope for this change.
AspNet.Security.OAuth.StackExchange – Has a Site option which specifies the SE site to query the user information from. Also used with the user information backchannel request.
AspNet.Security.OAuth.Instagram – Has a UseSignedRequests option which adds a signature to the backchannel request. To me, this is a one-time configuration which shouldn’t be overwritable.
AspNet.Security.OAuth.Amazon, AspNet.Security.OAuth.LinkedIn, AspNet.Security.OAuth.Vkontakte – These three have a Fields option which specifies what data to request from the UserInformationEndpoint  request. LinkedIn even has a special URL handling there. I can see these being useful to be overridden, however, in order to add new fields there, one would also have to add additional ClaimActions mappings to the Options object. So this actually won’t work on-demand and users should rather change the Options then (or add additional schemes).

So, tl;dr: I would actually only see a need for a single WeixinChallengeProperties type for the contrib authentication providers. So a type explosion would be avoided.

@poke thanks for taking a look! :+1:
A few notes, tho':

We've always deliberately avoided to have per-challenge customization due to the lack of a consistent story in ASP.NET Core. I can imagine this pattern will spread like fire as soon as it becomes supported by the "official" providers. In the providers I personally use, I can immediately think of a few ones you didn't list but that would benefit from that:

BattleNET has separate regions where user profiles are homed. It's currently only configurable globally but if we supported per-challenge selection, the end user could choose his region.
Dropbox has useful authorization request parameters like require_role, force_reapprove and force_reauthentication that only make sense per-challenge.
Wordpress has a blog parameter that allows selecting the blog you're granted an access to.

All the providers that support Fields would also benefit from this pattern: while you indeed have to add claim actions for the claims you need, these claim actions are "null-friendly", which means you can list all the supported claims in Options.ClaimsActions and control the ones you receive using the Options.Fields option, which could also be done per-challenge (in the LinkedIn provider, we've added more claim actions than the ones required for the few fields we request by default to make that easier for the developer).

Consistency is super important in the aspnet-contrib projects. If we were to add dedicated classes for at least 10 providers (the 7 you listed and the 3 I mentioned), we'd likely create classes for the other ones, for consistency.

@poke Supporting parameters for the userinfo request seams reasonable, it's all part of the login flow, these would just need to be persisted a little longer. Since these would actually require serialization across requests they'd need to be handled separately from @HaoK's Parameters suggestion.
@HaoK are you proposing Parameters as a IDictionary<String, Object> to avoid any serialization? That would simplify the Facebook scope formatting issue, the handle would do the formatting.

@Tratcher yes what I'm suggesting is:
AuthenticationProperties:

Items: Remains the same, anything that's using it behaves the same, this is what's serialized into state
- Parameters: This is a new IDictionary<String, object> that is never serialized, and is just metadata that can be passed around.

The main benefit is that each custom ChallengeProperties classes can decide the semantics/behavior of their properties (Items vs Parameters), and the right things will just happen.

Basically we are adding AuthenticationProperties.Parameters as a generic argument bag to challenge, which is what this PR is trying to do as far as I can tell...

If we have a IDictionary<string, object> bag that can hold any type of value, why do we need subclasses then?

Because strongly typed properties are much friendlier than having untyped Dictionary bags...

Because strongly typed properties are much friendlier than having untyped Dictionary bags...

That's very subjective. Dealing with untyped objects is only more complicated for the consumer that has to deal with casts. And here, the consumers are the handlers.
Also, how your approach will work with Identity's ConfigureExternalAuthenticationProperties API, that returns an untyped AuthenticationProperties?

But just to be clear, I'm not particularly attached to the idea of having subclasses, as long as the calling code looks something like:
context.ChallengeAsync("Facebook", createAProperties() { SetSomeProperties = "yay" })

Where you get intellesense for the supported parameters.

ConfigureExternalAuthenticationProperties  will likely need a new overload that uses an existing AuthenticationProperties instance instead of always creating one to enable challenge customizations

I personally don't like the subclass option much for one reason.
Based on my own experience, seeing await context.ChallengeAsync("Facebook", properties) in user code (with the scheme harcoded) is extremely unusual. People usually either make the external provider the default challenge scheme (and then amend the default parameters using the events API) or make the provider name dynamic:
await context.ChallengeAsync(provider, properties)

And in this case, it would be awful to see things like:
await context.ChallengeAsync(provider, new FacebookChallengeProperties
{
});

Or you wrap the AuthProperties after the fact.

Yeah, I like @Tratcher even better, since that requires no new overload, the app can just create the custom challenge after letting identity generate the initial one

@PinpointTownes except this whole feature is about dynamic overrides at the point of challenge, and those overrides are provider specific.

Yeah this feature isn't useful if you are normalizing all providers into a common API...you probably aren't trying apply provider specific challenge parameters if you are doing something like that

I thought this was only about how can I 'light' up and target specific parameters that one specific provider supports in their challenge.

@PinpointTownes except this whole feature is about dynamic overrides at the point of challenge, and those overrides are provider specific.

My point is that there's a risk of confusion. Folks might be tempted to think the properties set in FacebookChallengeProperties will only be honored if the provider is Facebook, which is not true. There's a real mix up risk if 2 different providers have the same property name for different purposes.

So you are worried about the fact that you'd likely be able to use FacebookChallengeProperties for google and vice versa?  Shrug, they both derive from OAuthChallengeProperties so I don't think that's too crazy...  It most likely wouldn't work if you tried to use OIDCChallengeProperties with OAuth though which makes sense right?

These challenge properties really are just there to scope the parameters down to a relevant set for the provider, without this, you'd basically just have constants all over the place, and the Challenge callee is left to their own mechanisms to figure out what parameter they should be passing in...

So you are worried about the fact that you'd likely be able to use FacebookChallengeProperties for google and vice versa?

Yes.

Shrug, they both derive from OAuthChallengeProperties so I don't think that's too crazy

That's certainly not a problem for the standard parameters, whose definition is strictly defined. That's a whole different story for provider-specific parameters, like Google's access_type parameter, that may have a totally different meaning in an aspnet-contrib provider exposing the same parameter.

@PinpointTownes well I think its safe to recommend that people don't try passing one providers properties to another provider...  The behavior rightly should be somewhat undefined and provider specific there...  If they happen to use the same parameters for different purposes...so be it, just don't do it...

The behavior rightly should be somewhat undefined and provider specific there... If they happen to use the same parameters for different purposes...so be it, just don't do it...

Thing is you may never realize that multiple providers support the same "properties".

The fundamental issue is that single challenge method is scheme agnostic, so AuthenticationProperties is as well. You get no type safety, its meant to work across all schemes.

That's pretty much my point: you're hiding this crucial detail behind a strongly-typed API that is "apparently safe".
At this point, I think I prefer @poke's initial suggestion (with a few tweaks) as it's the best compromise:
var properties = new AuthenticationProperties()
    // This only applies if provider is "Google".
    .OverwriteGoogleAccessType("access_type");

    // This only applies if the provider is an OIDC or OAuth provider.
    .OverwriteOAuthScope("scope");

await context.ChallengeAsync(provider, properties)

public static class Extensions
{
    /// <summary>
    /// Overrides the access_type parameter sent to Google's authorization endpoint.
    /// Note: this method doesn't have any effect when the challenge
    /// is processed by another provider.
    /// </summary>
    public static AuthenticationProperties OverwriteGoogleAccessType(this AuthenticationProperties properties, string type)
    {
        if (properties == null)
        {
            throw new ArgumentNullException(nameof(properties));
        }

        properties.Items[".google.access_type"] = type;

        return properties;
    }
}

That doesn't address the serialization issues.

you're hiding this crucial detail behind a strongly-typed API that is "apparently safe".

This really isn't a big deal to me, most people will never see this API at all.  The only way you see this API is if you are trying to do something custom with one particular provider...  so why would they think they can then suddenly take their FacebookChallengeProperties and pass it to BattleNet or something?

Everyone adding their own extension methods directly to AuthenticationProperties is a mess, and the current proposal is way cleaner...

That doesn't address the serialization issues.

How so? If you introduce an untyped Properties dictionary in AuthenticationProperties, there's no serialization needed.

so why would they think they can then suddenly take their FacebookChallengeProperties and pass it to BattleNet or something?

Because they'll do what everyone does: they'll copy an existing "challenge" MVC action (e.g taken from Identity). And most samples out there, the provider is dynamic.

Everyone adding their own extension methods directly to AuthenticationProperties is a mess, and the current proposal is way cleaner...

But at least, the fact the properties bag is shared between all the providers is immediately clear when you see all these provider-specific extensions.

This really isn't a big deal to me, most people will never see this API at all.

I guess the ultimate question is: if it's that hard to discover, does all this stuff really make user's life easier, compared to what's already supported in the OIDC handler? (the story is way more complicated in the OAuth handler, because you have to handle the redirection yourself if you want to customize the parameters).
Here's the currently supported approach (supported since Katana):
services.AddAuthentication()
    .AddOpenIdConnect(options =>
    {
        options.Events.OnRedirectToIdentityProvider = context =>
        {
            if (context.Properties.Items.TryGetValue("my_prompt", out var prompt))
                context.ProtocolMessage.Prompt = prompt;

            return Task.CompletedTask;
        };
    });

var properties = new AuthenticationProperties();
properties.Items["my_prompt"] = "login";
await context.ChallengeAsync(OpenIdConnectDefaults.AuthenticationScheme);

Are you arguing for us to just do nothing?  I'm certainly infavor of that if that's really what you are suggesting :)

My 2c, what we have today, may work, but its pretty hacky.  If we are going to apply this as a general pattern and expect it to ripple out, we will do it in a better/clean way...
That said, leaving it as is and waiting until 2.2 to see if more requests come in and giving us more time, seems like a fine option as well...

@HaoK if there's a low-risk way to substantially improve the situation, sure, a new API would be cool.
But if your argument is "well, the risk of confusion you're bothering us with doesn't exist because users won't see this new stuff", then it's not really an improvement as it doesn't really simplify things.

@PinpointTownes nah, my argument is its obvious you should pass the XyzChallengeProperties to the Xyz provider, and not the Abc provider.
I think the current proposal is fine, I think the previous intermediate/extension based ones are not

@poke what are you thoughts, this is your PR after all :)

My 2c, what we have today, may work, but its pretty hacky.

Out of curiosity, how is it hacky?

Using Items as an arbitrary grab bag where the handler then goes and deletes it from Items after seeing it (since it shouldn't be serialized like everything else) seems pretty hacky to me.

IMO the "grab bag" part is fine - but yes - needing to delete it because it otherwise becomes state is def hacky.

Using Items as an arbitrary grab bag where the handler then goes and deletes it from Items after seeing it (since it shouldn't be serialized like everything else) seems pretty hacky to me.

Well, if that's your only concern, it can be solved by either adding the AuthenticationProperties.Properties temp bag you suggested or by adding an helper to simplify that:
var prompt = context.Properties.GetAndRemove("my_prompt");

Yep, that was why I proposed that change.
The second part is coming up with a extensible recommended pattern for providers to expose their additional challenge properties.  Its a fairly natural and easy to implement pattern if providers just expose their own XyzChallengeProperties that expose whatever additional customization points they want.  I just view this as strictly better than a loose set of extension methods that are organized by naming convention instead of having off of a type...

@HaoK

@poke what are you thoughts, this is your PR after all :)

Wow, I’m away for a few hours and the PR completely explodes! 😆 Let me respond to what was said in order:
@PinpointTownes

We've always deliberately avoided to have per-challenge customization due to the lack of a consistent story in ASP.NET Core.

In that case, I guess we should be careful to think of a good solution that we can actually consider best practice.

BattleNET has separate regions where user profiles are homed. It's currently only configurable globally but if we supported per-challenge selection, the end user could choose his region.

To be honest, I don’t know about this one. The Battle.net region determines which authentication provider endpoints are used. These are completely different URLs. Even if OAuth application registration is still global, I consider this a gray area of customization which might benefit from having an actually separate authentication scheme.
I mean: If providers supported on-request customization of everything, where would we draw the line of what’s configuration of a single scheme and when is it appropriate to have a separately configured scheme? To me, it seems a lot clearer to have separate schemes then. That way, I could do this:
public async Task<IActionResult> Login(string region)
{
	await context.ChallengeAsync(region);
}

Instead of having to fiddle with the actual configurations there:
public async Task<IActionResult> Login(string region)
{
	await context.ChallengeAsync("BattleNet", new BattleNetChallengeProperties
	{
		AuthorizationEndpoint = RetrieveAuthorizationEndpointFromSomeOtherConfiguration(region),
		// …
	});
}

Of course, one could argue that you should just have to pass a single Region property then and the rest will be filled automatically; but then tell me why do we even have custom BattleNetAuthenticationOptions objects where we can manually specify the endpoint URLs? If we were to take the BattleNetAuthenticationDefaults as set in stone, then we wouldn’t need to allow that level of customization there.
I also understand the virtual authentication schemes that are coming with 2.1 as a general move into a direction where we would rather have users configure multiple schemes separately. So I would ask the question again from above: What properties do we actually want to make overwritable? Everything does not seem like a good idea; there are few ones that do not justify creating their own authentication scheme (the prompt still being a very good example).

Dropbox has useful authorization request parameters like require_role, force_reapprove and force_reauthentication that only make sense per-challenge.

Similarly to scope and prompt, these are good examples of what makes sense to specify on a per-request basis.

All the providers that support Fields would also benefit from this pattern

Supporting the Fields will require us to think of a completely different solution though. These are used in the user information endpoint query that happens after the user has successfully logged in with the authentication provider and gets directed back to the app, calling HandleRemoteAuthenticateAsync. At this point, the AuthenticationProperties object passed to the challenge request is no longer there. So properties we added to a new AuthenticationProperties.Parameters object are gone; we only have access to the state that has been serialized into the challenge request. So in order to support Fields, we would have to serialize them into the state.
In addition, one might argue that there is no actual need to customize this on a per-request basis. It makes sense to be able to change anything that would affect the actual challenge, the login prompt or a consent dialog a user gets at the authentication provider. Changing the scope or the prompt will actively affect what the user will see and what kind of permissions they have to grant. Specifying what properties to fetch after said challenge is already completed however does not really matter much. Worst case would be that you get more claims that you need for a particular challenge.

Consistency is super important in the aspnet-contrib projects.

For what it’s worth, while looking through the contrib providers, I noticed a lot code duplication. I think by properly abstracting some of the stuff, we could likely get rid of half of the handler implementations there. I personally don’t see anything wrong with a .AddFooBarAuthentication() call registering an actual FooBar scheme using an OAuthHandler as the authentication handler. After all, OAuth is a standard so that one could (theoretically) use one handler implementation with any OAuth authentication provider. So in an ideal world, the packages would just contain extension methods with the right default configuration.
@Tratcher

Supporting parameters for the userinfo request seams reasonable, it's all part of the login flow, these would just need to be persisted a little longer.

See above; while I agree that customization could be useful for certain providers, I usually wouldn’t expect this to be that useful though.
@HaoK

Parameters: This is a new IDictionary<String, object> that is never serialized by the state formatter.

While I would certainly welcome this, if only to make the handling of the challenge properties nicer, I do worry that having such a collection will raise certain expectations. After all, it would be very short-lived. E.g. for a challenge request, this would only affect the actual outgoing redirect, but not the incoming remote authentication request. Everything that would be needed there would still have to go into the Items collection to be serialized into the state. And whenever some custom code receives an AuthenticationProperties object, you can almost always be certain that it will be empty.
I haven’t looked at other authentication requests yet but are there actually use cases for other handler requests? E.g. SignInAsync() or ForbidAsync? If this ends up being just useful for challenge requests, and when we’re touching the AspNetCore.Authentication package anyway, we might as well expand the ChallengeAsync call to accept a third argument, with actual ChallengeParameters.
@PinpointTownes

Also, how your approach will work with Identity's ConfigureExternalAuthenticationProperties API, that returns an untyped AuthenticationProperties?

Regardless of what we end up with, I would still suggest that any solution that builds on top of AuthenticationProperties would still work with an untyped AuthenticationProperties object. So a consumer could use e.g. a SpecificChallengeProperties object; or they could use a plain AuthenticationProperties object with the right values being set.
So the authentication handlers wouldn’t exactly know about those specialized ChallengeProperties object and just access the items they need directly. That’s what I did in the current state of the PR, and I think that’s the most flexible way to do this: Allow specialized subtypes/extension methods/whatever to make it easier for end users configuring it, but still fall back, so that interoperability is guaranteed.
@HaoK

But just to be clear, I'm not particularly attached to the idea of having subclasses, as long as the calling code looks something like:
context.ChallengeAsync("Facebook", createAProperties() { SetSomeProperties = "yay" })

To support that, we would have to have a type containing those properties anyway, so I still believe that an actual class that gets newed-up there is the cleanliest way to do this (at least without ending up with a million extension overloads).
@PinpointTownes

await context.ChallengeAsync("Twitter", new FacebookChallengeProperties { … })

To be honest, I don’t think that’s a problem at all. If people attempt to do that, then it’s their own fault. As the others said, the whole scheme handling is dynamic anyway, so we cannot possibly verify that people will pass the right properties for the correct scheme. And right now, there is nothing that prevents people from doing this:
await context.ChallengeAsync("Twitter", new AuthenticationProperties(new Dictionary<string, string>
{
	["TotallyNotATwitterProperty"] = "hello world",
}));

So yeah, people could end up configuring properties that have no affect for the selected scheme; or they could use a different specialized object which happens to set a property with an accidentally correct (as in “meaniningful for the scheme”) parameter which could cause confusing behavior. But both those things are already possible now, and I would actually argue that having strongly typed objects having a clear name will more likely prevent that from happening.
Sure, using explicit provider-specific key names would help here (e.g. "OpenIdConnect.scope") but that would also make things more complicated when we want to share things. For example, would a OAuth.scope still apply for a Facebook provider? Why wouldn’t it be Facebook.scope then to avoid accidentally configuring my Facebook scheme with properties for my custom OAuth scheme? Or what happens when I have two OIDC providers? Will the prompt for the first be able to accidentally apply to the second? Or would the properties have to be scheme-name-scoped then?

I guess the ultimate question is: if it's that hard to discover, does all this stuff really make user's life easier, compared to what's already supported in the OIDC handler?

I’d say for the same reason why we are allowing users to configure things in the first place. The same reason they can configure OpenIdConnectOptions.Prompt now: Because it actually makes sense to modify these from time to time.
And for the same reason why aspnet-contrib exists: Surely all the users of those providers could just ship their own handler class and add the default configuration themselves. But it’s easier to just have a proper way of doing this instead of having to roll with your own implementation all the time.
So I guess the actual question is actually: Is overwriting some of the challenge properties an actual use case that users will encounter, so that it makes sense for us to invest time in building a proper solution? My answer to this is a clear yes, and from your comment above, I’d say that you agree.
We just need to make sure that we actually choose those properties that make sense to be overwritten, to not turn this into feature creep which will in the end just harm other and likely better solutions (e.g. multiple schemes of the same provider, as mentioned above).

So I would suggest that we take a step back and look again at what problems we want to solve at a larger scale (we’ve already gone far outside the original scope of the PR anyway :P):
It seems clear that we ideally only want to use AuthenticationProperties.Items for those items which will actually make it into the serialized state. That’s what the collection was made for. If we decide to put items on there which are non-serialized, then we should have the APIs for that on the object itself, e.g. AuthenticationProperties.GetAndRemove(key). Otherwise we could expand the properties with another AuthenticationProperties.Parameters collection that is explicitly for non-serialized parameters. We should check though whether we would only introduce this for the actual challenge request, or if this would have other applications as well.
If we end up just needing this for challenge requests, maybe expanding AuthenticationHttpContextExtensions.ChallengeAsync would be a good idea with the following overload(s):
ChallengeAsync(this HttpContext context, string scheme, AuthenticationProperties properties, ChallengeParameters parameters);
ChallengeAsync(this HttpContext context, string scheme, ChallengeParameters parameters);

And providers could supply their own ~ChallengeParameters type which would then end up being passed to HandleChallengeAsync where the provider could perform a runtime type check to only accept parameters of the right type (FacebookHandler could for example interpret both FacebookChallengeParameters and OAuthChallengeParameters).
This would make everything very clear and type-safe; we wouldn’t have a mess with property bags and lots of constants for the right key names. On the other side this would be a more explicit breaking change for custom authentication providers, although I believe that we can do this in a pretty compatible way just from editing the AuthenticationHandler to allow subclasses to implement either HandleChallengeAsync(AuthenticationProperties) or HandleChallengeAsync(AuthenticationProperties, ChallengeParameters) (so all existing providers would continue to work).
Currently, seeing how complicated this whole topic ended up being, this would be my favored approach. But I don’t know all the authentication handlers, nor Identity well enough to say whether this would work. So what do you think? Is there a use case for non-Challenge requests?

FYI AuthProperties are serialized across requests in the state field and available in HandleRemoteAuthenticateAsync

@Tratcher Yes, but only if we don’t explicitly remove items or decide to put the values into a new Parameters collection that is not serialized.

Adding parameters to ChallengeAsync is problematic/breaking, there are lots of places that information needs to flow through. Adding to AuthenticationProperties is our simplest avenue for expansion.

@poke I'm with you until the end.  Parameters should not be specific/tied to Challenges, if we are adding something to AuthenticationProperties it should behave in a consistent manner, and be available for use in similar ways in the other methods in the future if we need.
I this its pretty much a non-starter for us to introduce any breaking/major api changes to challenge or auth handlers in general for this feature (and basically this isn't happening in 2.1 at all if we go down this route).  The general idea (if we want this to make it into 2.1) should be to introduce this with a tiny footprint on the general auth public surface area, basically not all providers will want this functionality, they should't have to even be aware of this feature in that case.
The proposal I think that accomplishes everything is:

Add the AuthenticationProperties.Parameters dictionary property that providers can use for whatever they like.  The existing state formatters will ignore this, but there are no constraints on this bag, its just another bag just no current baggage/usage patterns.  But its intended purpose is to allow passing arguments to the handlers from the HttpContext callee, but that's just the pattern we use in our handlers, custom handlers should be free to deviate and use this bag for whatever they like.
For our handlers, add scheme provider specific XyzChallengeProperties subclasses that basically are just sugar on top of get/setters for constants Parameters specific to the provider, that the handler will use to allow per challenge customizations.  This is again just our opinionated implementation, derived handlers should not be forced to implement this pattern, and should be allowed to deviate and implement this a different way.
Leave any code that is currently using Items today alone, but also introduce the new ChallengeProperties as an alternate path to accomplish the same thing (i.e. Google) - basically no breaking changes.

Alright, those are valid arguments and make sense. – I think that sounds like a good plan then!
I guess the next step then would be for me to open a PR on the HttpAbstractions to expand the AuthenticationProperties object first? I would add the Parameters dictionary, and also use the opportunity to make the current private get/set helpers protected, so we could use them in subclasses.

Also, add an example here that would show a dynamic challenge like forcing Prompt:
https://github.com/aspnet/Security/blob/3fae140900fa837094c052b15a132816e0caa477/samples/OpenIdConnectSample/Startup.cs#L100-L108

@poke yup, open a HttpAbstractions PR

For what it’s worth, while looking through the contrib providers, I noticed a lot code duplication. I think by properly abstracting some of the stuff, we could likely get rid of half of the handler implementations there.

I'd love to know where :+1:
The only part we have to customize in ALL the providers is the userinfo request, which is not standardized in the OAuth2 world and can't be abstracted in a common implementation (everyone does it differently).

But both those things are already possible now, and I would actually argue that having strongly typed objects having a clear name will more likely prevent that from happening. And right now, there is nothing that prevents people from doing this:

Except none of the built-in providers - except Google - has currently code that does something special with the authentication properties, so your snippet has no effect and you can't shoot yourself in the foot in 1.0/2.0: if you want your properties to have an effect, you have to add code via the events model to handle them. This won't be true tomorrow.

So I guess the actual question is actually: Is overwriting some of the challenge properties an actual use case that users will encounter, so that it makes sense for us to invest time in building a proper solution? My answer to this is a clear yes, and from your comment above, I’d say that you agree.

Sure. But let's be pragmatic: it's way less pressing for OIDC than for the OAuth2 generic handler, whose RedirectToAuthorizationEndpoint event doesn't allow adding/replacing/removing a parameter easily. If there was an easy way to customize that, these new APIs wouldn't be so appealing :smile:

I’ve pushed an initial attempt that builds on top of the PR in the HttpAbstractions repo. That ended up working quicker than expected.

Looks good to me, very nice!

I'll give it a more thorough review tomorrow, but so far so good.
Also, thanks for putting up with all of this. Not only do you have the three of us pecking it to death, but you even figured out the cross repo changes on your own. We have trouble with those on a good day. Nicely done.

@Tratcher Thanks! It’s been quite a ride, but also a very interesting experience! I’m glad that we ended up in a direction that’s working for everybody now.
And yeah, does it help to mention that the tooling for cross-repo work sucks? Especially compared to the old DNX days 😆

Believe me, we know...

Rebased onto release/2.1 to make this compatible with the HttpAbstractions PR and addressed some of the remaining feedback.
The open questions for me right now (before I’ll finalize this and cleanup/squash the commits):

Do we keep the new public API for AuthenticationProperties now? I.e. the Get/SetString and Get/SetParameter<T>? Do we want to change the visibility? Do we want to make GetParameter<T> have a second T defaultValue argument?
Is the FormatScope() vs. FormatScope(IEnumerable<string>) situation good enough to maintain as much compatibility as possible with existing subclasses?

I see the words "breaking changes" have been mentioned multiple times in this thread.
I'd like to emphasize that even behavioral changes would be intolerable. We don't have plans to migrate the aspnet-contrib providers to 2.1 (I just can't maintain one version for LTS and one for the "current" ASP.NET Core bits) so the providers written for 2.0 must keep working exactly the same when referencing the 2.1 packages.

There's something I don't quite follow with the new AuthenticationProperties.Parameters bag: what's the point of introducing it if all the overridden parameters are ultimately stringified when building the OAuth2/OpenID Connect message? Couldn't the "parameters" be directly stored as strings in AuthenticationProperties.Parameters by the subclasses setters?

But its intended purpose is to allow passing arguments to the handlers from the HttpContext callee, but that's just the pattern we use in our handlers, custom handlers should be free to deviate and use this bag for whatever they like.

Since the aspnet-contrib providers always target LTS, we won't be able to use this new bag anyway. If we decide to support per-challenge properties, we'll have to use the existing AuthenticationProperties.Items.

With the Parameters the serialization is all centralized in the handler. It knows best what format will be required. It can also better reason about the object (e.g. read a date) before serializing it.

It knows best what format will be required.

Why? Since all the subclasses are provider-specific, it wouldn't be unreasonable to make them match the provider format.

Look at the Facebook derived class we were able to remove. Facebook customizes the Scope formatting but you don't need a whole properties type for that, you can use the base OAuth properties to flow the raw list and let the Facebook handler format it.

Hum, I thought you wanted to make the "fields" customizable? How are you going to do that without the Facebook subclass?

As for breaking changes, I agree, each mention was to prevent any. Redit may not get to use the new scope override pattern until it's updated, but it won't break.

We'll see if we get as far as facebook fields. Regardless, the less work the properties classes do, the fewer you need.

Regardless, the less work the properties classes do, the fewer you need.

Calling string.Join in a setter doesn't seem to be a lot of work. And if AuthenticationProperties.Items was used instead of this whole new bag, the OAuth2 providers written for 2.0 (including the Weixin, Weibo and Reddit aspnet-contrib providers) would be completely compatible with the new "scope" override thingy without any change on our side.

It's worse when you start getting into complex types like dates.

It's worse when you start getting into complex types like dates.

That would be a valid point if we had "dates" in OAuth2/OIDC (at most, we have timestamps, which are numbers).
Don't forget everything is ultimately represented as a query/string string: there's really no point introducing complex types in the properties.

Don't forget everything is ultimately represented as a query/form string: there's really no point introducing complex types in the properties.

That's not the point of the new Parameters feature, its meant to be a general purpose mechanism to pass parameters to the handler.  They aren't limited to query/form string, that's just what this feature is using the bag for.  But the idea is you could use this bag to pass arbitrary data to Authenticate/SignIn/etc as well.

And if AuthenticationProperties.Items was used instead of this whole new bag, the OAuth2 providers written for 2.0 (including the Weixin, Weibo and Reddit aspnet-contrib providers) would be completely compatible with the new "scope" override thingy

Since this is a 2.1 feature, I think its totally reasonable for any providers to 'move' to 2.1 to take advantage of this

I think 2.1 is expected to be new LTS anyways so you won't have to worry about being stuck on 2.0 for aspnet-contrib

@PinpointTownes

I'd like to emphasize that even behavioral changes would be intolerable.

This change is designed to be completely non-breaking. All existing handlers will continue to work as it is. The only thing that affects a few handlers is that they will not support overwriting the Scope from the AuthenticationProperties object. This applies to those handlers that don’t require a custom non-standard scope separator.
But I don’t consider “new feature not working for an old handler” a breaking change. It’s just that those will not automatically upgrade to the new functionality (while all other ones will).

Couldn't the "parameters" be directly stored as strings in AuthenticationProperties.Parameters by the subclasses setters?

I did not want every provider to have to define their own AuthenticationProperties subtype. They can, but they don’t have to. And all OAuth based providers are free to reuse the OAuthChallengeProperties type; including those that require a different scope formatting.
If we were to format the scopes within the properties object, then we would also have to duplicate the login within the properties when it’s already in the handler. Going this route really means less work for custom handlers.

Since the aspnet-contrib providers always target LTS, we won't be able to use this new bag anyway.

That’s really a shame; but almost all providers will get the Scope overriding capabilities anyway if they use 2.1.

Hum, I thought you wanted to make the "fields" customizable? How are you going to do that without the Facebook subclass?

That’s not and never was part of this PR. We were just talking about it in the context of looking at the contrib providers, in what customization capabilities might be there.

Calling string.Join in a setter doesn't seem to be a lot of work.

But it’s still provider-specific logic. Logic that doesn’t have to be spread across multiple types

And if AuthenticationProperties.Items was used instead of this whole new bag, the OAuth2 providers written for 2.0 (including the Weixin, Weibo and Reddit aspnet-contrib providers) would be completely compatible with the new "scope" override thingy without any change on our side

That’s already how this change works. All OAuth providers that don’t override FormatScope() will work and support the scope customization. Those that override FormatScope() will not and also wouldn’t work if we used the Items collection instead. The thing that will break those handlers is that they don’t support passing the overwritten scopes to FormatScope.

Don't forget everything is ultimately represented as a query/string string: there's really no point introducing complex types in the properties.

As per that other PR on HttpAbstractions, the Parameters bag is an all-purpose bag for passing things to the handlers. Customizing OAuth authentication redirects is just one primary application that we will use in the built-in handlers. But it’s not limited to that.

But I don’t consider “new feature not working for an old handler” a breaking change.

I'm fine with that, even if that's unfortunate.
When reading @Tratcher's messages, I was afraid your code would introduce a behavior change by making our FormatScopes() overrides no-op (which would be terrible since our providers would no longer work).

That’s not and never was part of this PR. We were just talking about it in the context of looking at the contrib providers, in what customization capabilities might be there.

Obviously my remark was not limited to the current state of this PR. It's always good to have a long-term vision :sweat_smile:
If customizing fields is desirable, then a FacebookChallengeProperties will be needed at some point.

Those that override FormatScope() will not and also wouldn’t work if we used the Items collection instead.

They definitely would if you used the scopes item property when present instead of calling FormatScope(), which is exactly what the Google provider is currently doing. But that's obviously not the direction you're heading to.

That’s really a shame

Maintaining 59 packages is big task for a single man. No one from the ASP.NET team has even offered to help with that so I can't reasonably release a new version each time they decide to push a new release.
(even the migration to 2.0 has been extremely painful. Luckily, we have awesome contributors who've taken care of porting all the providers, one by one).

FWIW, as long as there's no breaking change, I'm fine with these changes (even if I would have preferred a solution that allowed overriding scopes in providers written for 2.0).

No one from the ASP.NET team has even offered to help with that so I can't reasonably release a new version each time they decide to push a new release.

Just to be clear: it's a fact, not a criticism :trollface:

@PinpointTownes Maybe @poke wants to come help you in contrib? He's done great here.

@PinpointTownes If you wanted to add something like an aspnet-contrib canary provider to 'share' the pain so to speak, you could submit a PR adding something to AuthSamples like an 'example' aspnet-contrib, so if you make us go thru some of the upgrade pain, its likely it will be less painful :)

@PinpointTownes Maybe @poke wants to come help you in contrib? He's done great here.

I can't speak for him, but if here's interested, I'm sure we'll have a chance to talk about that on Slack :clap:

Assuming the process is different than what we do for our in the box providers...

Oh, it's not. When you decided to break everything with your "auth 2.0" thingy, you had 4 OAuth2 providers to migrate (which you did as part of your day job).
When we had to do the same thing, we had more than 40 projects to port on our spare time (and I don't even mention the other aspnet-contrib projects like ASOS or the OpenID 2.0 generic provider).
I'm sure you can see where the pain was.

@HaoK Thank you!
I’ll wait for a final word from @Tratcher, and then I’m going clean up this commit mess a bit… and I guess then we’re ready 😁

I wouldn't worry about cleaning up the commits etc, we typically squash before rebase/merging anyways

Whaaat, you squash? 😨 Let me at least split up the test cleanup from the actual implementation, and author a proper commit message 😭

Feel free to fix your commits then :)

but yeah generally we like to have one commit for a feature

Cleaned everything up and rebased to current 2.1!

And merged to dev & release. Thanks for the hard work everybody.

Thank you everybody! This has been an incredible and fun experience!


Extending applicability and utility of the c0apps.

I wanted to suggest extending the m0tr demo apps in such a way that they could be extended and leveraged without necessarily runtime binding to m0tr or even "C" libraries.
For better or worse, most of the tool and utility world has moved on from C, C++, and even Python to "go".  As you probably know, it is the foundation for most of Kubernetes, OpenDNS, etcd, MinIO, Prometheus, and nearly every interesting paradigm shift in the open source cluster world.
Creating golang binding to the m0tr interfaces demonstrated in the c0apps can be done, but it would take a couple of weeks to do and would probably reflect a layer of coupling to m0tr implementation details that I'd really rather not take on.
The stupidest, simplest thing that could possibly work is to leverage named pipes or some such as a data IPC conduit between the m0tr world and demo m0tr applications. This would render them language agnostic and enforce a line of demarcation that would allow m0tr to continue to improve independent of applications demonstrating and leveraging it.
If we refactored the c0apps to accept a named pipe to sink/source data streams, and where appropriate, render json informational inputs and outputs, it would be very easy to plumb in a prototypes of other use cases.  If the prototypes demonstrate potential value, then the effort to develop an optimized interface could be invested.

I Agree @suykerbuyk. The current implementation of c0apps started as sample implementation to demonstrate Clovis (client) interface EU project partners. It still serves as source code examples to new comers to m0tr client interface. A go implementation was recently made available here:
https://github.com/Seagate/cortx-motr/tree/3c3921704f18544a181592aaab4cd7bffbc5b202/bindings/go
This provides a utility called mcp that reads/writes files to/from m0tr objects. Again, mcp is a simple demo utility but I have seen the power of it when we needed a utility to read/write to/from the Sage cluster using multiple threads.
Certainly we should explore go option more.

Thanks all.  Slightly better link to the go bindings: https://github.com/Seagate/cortx-motr/tree/main/bindings/go.
🙂

From: Ganesan Umanesan<EMAIL_ADDRESS>Sent: Friday, February 26, 2021 10:39 AM
To: Seagate/cortx-motr-apps<EMAIL_ADDRESS>Cc: Subscribed<EMAIL_ADDRESS>Subject: Re: [Seagate/cortx-motr-apps] Extending applicability and utility of the c0apps. (#23)
I Agree @suykerbuykhttps://github.com/suykerbuyk. The current implementation of c0apps started as sample implementation to demonstrate Clovis (client) interface EU project partners. It still serves as source code examples to new comers to m0tr client interface. A go implementation was recently made available here:
https://github.com/Seagate/cortx-motr/tree/3c3921704f18544a181592aaab4cd7bffbc5b202/bindings/go
This provides a utility called mcp that reads/writes files to/from m0tr objects. Again, mcp is a simple demo utility but I have seen the power of it when we needed a utility to read/write to/from the Sage cluster using multiple threads.
Certainly we should explore go option more.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHubhttps://github.com/Seagate/cortx-motr-apps/issues/23#issuecomment-786791490, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAPT2PQA7VNQRNEDAM56HQTTA7METANCNFSM4YIYXUIQ.

Chandradhar Raval commented in Jira Server:

Marking this Jira Closed based on above comment


New object allocation for every item added to adapter

Is this the best idea?
Can we reused items?
I am doing this for now:
private List<MyItem> getItemsFor(objects) {
  Lists<MyItem> output = Lists.newArrayList();
  for (MyObject object: objects) {
    output.add(new MyItem(object, stuff, param, moreStuff));
  }
  return output;
}

The reason of my concern is that the place where I'm trying to use does too much upon bindView and in  constructor.

@shubhamchaudhary if you want to have your model split of the items you can use the GenericItemAdapter which does things like this for you.

EDIT: the GenericItemAdapter will no longer keep an additional list of the model


Fix for bad middleware checks when wrapping them on load

This fixes the issues we were having yesterday. Let me know what you think.
Ideally, I'd like to completely get rid of _is_async. We can discuss a bit more about how this can be done today. We may need to explore the different ways of writing middleware and check the appropriate method/functions in them

Agreed that _is_async can probably go. It appears that we can derive the same information by whether the __call__ of a middleware is marked as async, so having a duplicate signalling mechanism isn't necessary.


Need help on subtotal  How to Insert subtotal row

Need help on subtotal,  How to Insert subtotal row in pivot

hi, Have you made progress in this ? I also need this feature.

This isn't actually on our roadmap at this time :)


Snapdragon Flight very high IMU noise and bad attitude estimation

I'm having the problem that the Snapdragon Flight's attitude estimation is performing very badly.
The snapdragon is mounted on a Bebop frame in a way that it takes advantage of the dampers that the Bebop has (although I'm not convinced they actually dampen the frequencies we need them to).
In flight, IMU measurements are extremely noisy, up to +/-100m/s^2, see this log: http://logs.uaventure.com/view/YbxWqciEHWJPFP8U4BRZYA
This was with the higher sampling of https://github.com/PX4/DriverFramework/pull/57.
The quad's performance in air is unsurprisingly quite terrible, the attitude seems to drift off significantly.
I have a few questions:

With the high sampling rate of 4kHz, are the measurements being down-sampled?
If they are, are they being filtered or simply windowed integration?
What other possibilities do we have to improve the measurements besides complicated mechanical damping? It seems this shouldn't be necessary, other platforms work even completely hard-mounted.

A likely related issue is that the attitude estimation is very poor even when the quad is stationary and mag fusion is off.
While my quad is stationary, I have an estimated yaw speed of about 20 deg/sec. I have uploaded a log here: http://logs.uaventure.com/view/UCmmvsKeorVQHENfYBWyZQ
I have recently calibrated the IMUs, but the mag calibration fails with calibration failed: reading sensor (I can see the mag measurements in QGC without a problem, though). Because of this failed mag calibration I disabled mag fusion in the EKF2 with EKF_MAG_TYPE = None.
With EKF_MAG_TYPE = Magnetic heading, the situation is much better. But it appears to simply be able to cover up a significant problem with the attitude estimation.

Thanks for reporting. I just did some thinking about the integration happening and I discovered a major flaw that I need to fix:
8 consecutive measurements are more or less published with the same timestamp. This means only every 8th dt is >> 0 and therefore we probably only really see every 8th sample.
I'll fix this next up.

Julien, I have a couple of questions about the integration method:

Are you using trapezoidal or rectangular integration?
What delta time do you assign to each trapezoid or rectangle?

@priseborough I'm working on the integration thing right now.

I think it is trapezoidal: (val + _last_val) * dt * 0.5f
The correct one :), at least with the changes I'm pushing soon.

@ndepal Is this fixed?

I'm still seeing a very high IMU noise band:
http://logs.uaventure.com/view/sBGdZuKZoyPHDCZPg4Wr8K
What IMU values are being logged exactly? The ones integrated/downsampled to 250Hz? Or simply a subsampling of the raw values (i.e. taking every Xth raw value)?

Your link doesn't work for me - is this the right link?

Yes, that should be the right link. Although I'm not seeing anything either.
Copy-pasted from the email:
Plots: http://logs.uaventure.com/view/sBGdZuKZoyPHDCZPg4Wr8K
Log: http://logs.uaventure.com/logmuncher/charts/sBGdZuKZoyPHDCZPg4Wr8K/logmuncher.log

@ndepal Yes, what you see in the log is not what ekf2 uses. You see 1/32 samples while the ekf2 see's the sum of 32 samples weighted by the dt (which should be pretty constant).

With https://github.com/PX4/Firmware/pull/4340 the in-flight performance issue is solved, attitude control is quite good now (confirmed by @tumbili). The attitude drift without mag fusion (second log in original post) is still the same.

@priseborough @tumbili Could you look into the attitude drift when mag fusion is disabled? We would expect only a slow yaw drift, not a fundamental issue. This is very relevant for urban environments and it would be very helpful to resolve.

I've replaced this issue with https://github.com/PX4/ecl/issues/108

I have the same issue in v1.4.1rc2. The noise of accelerometer is very high.
Plots: http://logs.uaventure.com/view/qBkzZ9VQunNuRRsB6tGNYU#IMU_Acc_PLOT
Log: http://logs.uaventure.com/logmuncher/charts/qBkzZ9VQunNuRRsB6tGNYU/logmuncher.log
I'm also trying to mount Pixracer that only enabling the MPU9250 beside the snapdragon flight. But the waveform in sdlog is very smooth.


Redirect after session expire

I made this PR to customise the redirect after session expire.
The default stays the same to invalid_session_path, but it is now possible to set your own route in the config.

Great! Can you look through dce_lti/spec/controllers/dce_lti/sessions_controller_spec.rb for examples  (and probably other spec files) and think of a test for this feature?

I will look at the test later this week.

I added a test for the redirect_after_session_expired.
The test is based on 'redirects to "redirect_after_successful_auth" url when it is a proc' in sessions_controller_spec, but I put the test in posts_controller_spec, because I wanted to request posts index without a valid lti session, which than should redirect me to the url specified in the redirect_after_session_expire proc.
I fixed some deprecation warnings while adding a test.
Please let me know what you think of the test.

This looks good - can you squash your commits, I'll give it one last look and merge? Thanks so much!

I squashed the commits.

Thanks! I'm going to look at this (along with the newly activerecord_session-store release) and merge soon.


Fix pub cache issue in Windows

Make sure that the PUB_CACHE is set with back-slashes on Windows
Fixes https://github.com/dart-lang/setup-dart/issues/32

I think this might fix that other pub issue you had with setup-dart recently?


chore: bdd-spring-1574593107 to 0.0.1

chore: Promote bdd-spring-1574593107 to version 0.0.1

[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers:
If they are not already assigned, you can assign the PR to them by writing /assign  in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment


Add border-radius to textarea element

Hi
I noticed that border-radius was consistent in Form and Input elements, but it was missing in the textarea element. So I added it to eccentric.css file. I set the border-radius to 4px, which is the same as of other elements.
Here, you can see the output:
Before:

After:

It'll be great if you can merge this PR!

There's an issue @guptasajal411, Your code changes are fine but you have raised a PR on the master branch, try again with the v2.0 branch. That would be great.

Also, We would really appreciate you if you give a star to this repo ⭐ 😄 👍🏽

Yes sure, I will close this branch and create a new PR on v2.0 branch. Also, I starred this repo 😄

Also, If you want to join our community on discord then feel free to join! You're invited to The DesignSystems
Join the Discord Server here


Stop deleting old or closed Issues...

Please, stop deleting old or closed Issues, which I found to be the best source of learning and understanding Blazor. I'm certain that many developers like me keep a list of bookmarks, which they reference time and again.

So we recently consolidated all our repos in the aspnet org to a few repos and in doing so a bunch of links to older issues broke ☹. At least to my knowledge, we never delete issues on github so in theory the issues you are referring to should still exist in the aspnet/AspNetCore repo. Sorry for the inconvenience though!

Good to hear that... Thanks a lot, mikaelm12

We have a label for the Blazor issues. Here is the query link
https://github.com/aspnet/AspNetCore/issues?utf8=✓&q=is%3Aopen+is%3Aissue+label%3Afeature-blazor+
Does this contain the issues you were thinking of @A51UK ?

Yes thank you.

@mikaelm12: Does this contain the issues you were thinking of @A51UK ?
No, it does NOT... Many Issues cannot any longer be accessed. I've dozens of bookmarks to these important issues, though closed, are of immense importance to me, and yet are not available. I've trusted Github, but not any longer. And from now on I store important issues and samples on my computer.


Automatic DB creation

Added sink option AutoCreateSqlDatabase
Lots of refactoring (removed duplicate code, cleaner interfaces of sink dependency classes, etc.)
Unit tests
Bumped version to 6.3.0

@jonorossi No worries. It was not urgent and there were quite some code changes to review in this PR.
I agree. This feature is not something I would typically use in production either. For me it is more something that helps in training or technical research scenarios where someone would quickly create a sample app with the sink and test it locally on a dev machine with a local SQL Server Express without having to create the DB as an extra step. But according to issue #360, there seems to be some real world demand for it, it was easy to add and is disabled by default.


UdpSockets.bind was called with 4 arguments but expects 5 arguments.

iOS
react-native 0.50.3
react-native-ntp-client 0.5.5
react-native-udp 2.3.1

I got this error
UdpSockets.bind was called with 4 arguments but expects 5 arguments. If you haven't changed this method yourself, this usually means that your versions of the native code and JavaScript code are out of sync. Updating both should make this error go away.

https://github.com/tradle/react-native-udp/issues/59
I see this issue that said it's fixed in 2.2.1 but it's still error for me.
It's work fine in android but crash in iOS
how can I fixed this, Thank :)

can you pause it in the debugger on this line https://github.com/tradle/react-native-udp/blob/master/UdpSocket.js#L99 and paste the local variables?

sorry @mvayngrib I'm not sure how to do that ;(

@hatyaiapp if you push your project and give me a link, I can take a quick look

closing due to inactivity

I have the same issue.
@mvayngrib in the line you asked for I see only 4 arguments passed. Is that the issue?

iOS
react-native 0.56.0
react-native-ntp-client 0.5.5
react-native-udp 2.3.1

This is still a problem, why is this closed?

@Anarchonist7 only one person hit it and couldn't provide a repro or additional information. If you can, happy to reopen

@mvayngrib I appreciate the quick reply. I'm in the exact same situation as that other guy. I'm beginning to think it's not an issue with this particular package though, but rather react-native-clock-sync. react-native-ntp-client works fine, and as I understand, it also depends upon this package. That one was updated 16 days ago though, so clearly they fixed whatever the problem was. This project is complete new, completely bare, created it last night exclusively for the purpose of finding a clock sync that works. react-native-ntp gets me a unix time in milliseconds, which is what I want, but it doesn't keep a clock ticking to that time offline, which is really what I need, and the functionality that is in react-native-clock-sync.
Should I make a repo for it and post it here? There's literally nothing in it other than the modules listed above. Once again thank you for the timely reply, and would appreciate any advice for solving my issue of needing an in app clock synced to unix time

@Anarchonist7 I've been facing same issue, I'm using react-native-clock-sync too.
Did you figure out the solution to fix it?

Please, annny news on this issue?

Only iOS

Any updates on this? Also struggling with it atm

I think I fount the cure, but it's very silly. react-native-clock-sync also depends on react-native-ntp-client
I've tried manually installing it to the project, and it works now as expected.
yarn add react-native-ntp-client
npm i react-native-ntp-client
Hope it helps anyone :)


Add references count to SearchableGroupListPopup

The SearchableGroupListPopup is used to assign groups to components in multiple places (ProcessSceneObjectEditor, ProcessSceneReferenceDrawer, HasGuidValidationEditor, LockableObjectsDrawer). Having many Tags in a project will make it hard to find the correct Tag especially if there are multiple scenes using different groups.
Sowing the number of times the group is used could help. Rework the SearchableGroupListPopup, SearchableList and SearchableListItem and the so it looks similar to the group list in the ProcessSceneReferenceDrawer
SearchableGroupListPopup.cs / SearchableList.uxml / SearchableListItem.uxml

ProcessSceneReferenceDrawer.cs/ ManageGroupsPanel.uxml

Nice to have:

Make the Parmaers VisualTreeAsset searchableList, VisualTreeAsset listItem in the constructor of SearchableGroupListPopup optional and and use VisualTreeAsset searchableList = ViewDictionary.LoadAsset(ViewDictionary.EnumType.SearchableList); VisualTreeAsset groupListItem = ViewDictionary.LoadAsset(ViewDictionary.EnumType.SearchableListItem);
as default values if unassigned.
Remove the Parameters from the calls to SearchableGroupListPopup in (ProcessSceneObjectEditor, ProcessSceneReferenceDrawer, HasGuidValidationEditor, LockableObjectsDrawer)

Internal tracking reference MP-2532


Task 65/fix near wallet signing with attached near

Looks like there's very similar changes here to #70. Can we address the comments there first. Should hopefully isolate what exactly this PR has changed in comparison too.

you're right, that other PR also solves that issue for me regarding needing a redirect to Wallet when attaching some NEAR


🌐 Add Russsian translation

I could have made a mistake in the names of variables and so on

Hi @CMAULTOP
There have been recent additions to these documents, so please update the translations you have provided, and thank you for submitting your PR!


Support TlaFunOper.{funDef,recFunDef} in PBT

#1401 disabled TlaFunOper.{funDef,recFunDef} in all property-based tests for performance regression on CI.
It should be re-introduced wherever possible. Use this issue to track progress.
@konnov suggested in https://github.com/informalsystems/apalache/issues/1401#issuecomment-1048894262 that this may only affect recursive functions.

Reproducing ScalaCheck runs: ScalaCheck PR, tutorial


How is Connexion doing in production?

I'm considering using Connexion in production for similar reasons Zalando's blog post outlines.
I have a couple of questions before actually doing that, none of which are actually issues, but this seems like the best medium to get in touch with the team behind this project to help shed some light here.
Given that the post is from January (but commit history goes back a year), I'm interested in how Connexion has served Zalando in a production environment:

Is this project used in production?
If so, how is it performing?
How many requests does it handle over a given day (or hour)?
Do you have percentile breakdowns of the time it takes to service a request?
What's the progress on #124? It seems that the only way to implement our own authentication is to decorate functions explicitly.

If other organizations use this in production, I'm interested in hearing your thoughts on how it's served your production environments as well.
Thanks.

We are using it in "production" at Zalando, at least for low-traffic services (not anything customer facing). One example is Lizzy: https://github.com/zalando/lizzy (both traffic and latency do not matter in this case)
I'll try to find out what use cases Connexion is used for in Zalando...

Thanks for the quick reply, @hjacobs. Oh, okay. Thanks for digging up more use cases / data on it.

This is a bit of an aside, but because of your experience with API First / Swagger-first services, what project is running your customer-facing API gateway?

@nickdirienzo e.g. our spark-appliance is using connexion as a http interface for handling request in a restful manner: https://github.com/zalando/spark-appliance
several teams are using this for production usecases, mostly in analysis and reporting. again: latency, traffic and speed do mostly not matter here. but connexion is serving us well there ... @chutium can tell you more about it ...

Hi @nickdirienzo , yes, our team is using Connexion for some low-traffic services, non-customer-facing API, such as the spark-appliance mentioned by @drummerwolli , there is a REST API built with Connexion (https://github.com/zalando/spark-appliance/blob/master/webapp.py) for providing an OAuth/JWT protected gateway for spark cluster.
but yes, latency, traffic or speed are no matter here, all of REST API call to this spark-appliance are async.

@nickdirienzo here the other API First frameworks (JVM based) we use: https://github.com/zalando/restful-api-guidelines/blob/tooling-section/tooling/Tooling.md

Ah alright. Thanks for the info on how you use Connexion internally, @drummerwolli and @chutiu. And thanks for the extra info on API First frameworks, @hjacobs!

Great thread, thanks for sharing. Has anyone done a benchmark to see the real cost of connexion?


Only disable namespace lists for cluster scope in constraints

Switched the LabeledSelects to use the tooltip rather than having a separate header and <v-tooltip> now that it's supported.
Fixed an issue with <Select> not having a min-height.
Fixed an issue where <LabeledSelect> hover tooltips caused the menu to go behind the :multiple selection of other LabeledSelect

rancher/dashboard#1892

@westlywright I didn't notice anything break when removing, if you remember what it was specifically for let me know and I'll test. But this code resulted in hover-tooltip causing the menu to go behind the :multiple selection of other LabeledSelect. Tooltips still seemed to behave but maybe they overlapped something it shouldn't elsewhere?
&.hoverable ::v-deep {
    .v-select * {
      z-index: z-index('overContent');
    }
}


[5.7] add support for at notation for termination callbacks

In fact it is already supported, since it uses the $app->call(... to invoke the callbacks.
we can widen the accepted parameters types to whatever the call method accepts.
And this change is going to be backward compatible.

Whilst unlikely - at the purest notation this would be a breaking change as your changing the signature of a function. Anyone extending that function (although unlikely) would have it break...?

Oh yeah, nice tip thanks


Implementing onSubmitted for Material SearchBar

This PR aims at fixing #126551.
At this moment in time I am not able to run tests because I receive a dependency conflict when running flutter pub get. I am not sure how to solve this (I'm surprised I even get an error when running pub get in packages/flutter).
Pre-launch Checklist

[x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
[x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
[x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
[x] I signed the [CLA].
[x] I listed at least one issue that this PR fixes in the description above.
[x] I updated/added relevant documentation (doc comments with ///).
[ ] I added new tests to check the change I am making, or this PR is [test-exempt].
[ ] All existing and new tests are passing.

Thanks again for your contribution! The fix(#129365) is merged now, so I'll close this PR because of duplication.

Thank you instead. I'm sorry I never found time to address this.


Fix panic for empty line in script tag containing only a tab (or few spaces)

This fixes an error I discovered by formatting this snippet containing a mix of 2 and 4 spaces indents with markup_fmt.
<head>
    <style>
      .foo .icon {
        height: 32px;
        width: 32px;
      }

      .foo {
        margin: 0;
      }
    </style>
</head>

The first pass lead to this:
<head>
    <style>
    .foo .icon {
      height: 32px;
      width: 32px;
    }
-
+
    .foo {
      margin: 0;
    }
    </style>
</head>

And the second one panicked because of the 2-space indent between the css classes

There's a single space in the <style> tag of the tests. Is this expected?

Wasn't expected (tho the error exists for any number of space smaller than the indent width). Got probably scammed because I opened the file in intellij after editing in nano.
This should be fixed now!


Parameters not shown in response url

The documentation says you can inspect the response url attribute to see that the query parameters were encoded correctly, but this does not seem to be the case:
$ cat tmp.py
import asyncio
import aiohttp

def run():
    payload = {'key1': 'value1', 'key2': 'value2'}
    r = yield from aiohttp.request('get', 'http://httpbin.org/get', params=payload)
    print(r.url)

asyncio.get_event_loop().run_until_complete(run())
$ python3.4 tmp.py
http://httpbin.org/get

I expected it to print http://httpbin.org/get?key2=value2&key1=value1

Thanks for bug report.
We should override .url in ClientResponse.update_path as well as self.path.
@danielnelson would you prepare Pull Request with code and test?


auto generate endpoint docs & serve

Resolves #13

Currently image and background_image returns the entire encoded image which makes API response hard to read and also makes response time slow if you are not going to need the image.
Maybe it's better for image to simply return the /avatar/{name} ?


Latest release 1.112 fails to run on Mac.

On a Mac running macOS Monterey Version 12.6 the command returns
apigeecli -v
Killed: 9

did you install via the downloadLatest script? What is the processor type?

Both via the installer script and manually.
apigeecli_v1.112_Darwin_x86_64/apigeecli

+1 - I see the same issue
the Linux version seem to work
The issue with the MacOS is - cannot be opened because it is from an unidentified developer

No, that's not the issue.
If you right-click on the executable, then select open in the Finder, then close, you'll then be able to run it.
Or try to run it.
Nandan is working on it.

The problem was upx. I'm using version 3.96, which also happens to be the latest. I have removed upx for all version (even though it worked for linux.
@ssvaidyanathan confirmed it works for him. Please try this binary. I may have a new release v1.113 since this is impacting many users.

Already checked, it works fine.
Regards,
Kurt
Kurt Kanaskie
apigee https://apigee.com/api-management/ | Technical Solution
Consultant - Specialist CE - BAP CoE
@.***
484 695 1106
On Tue, Oct 11, 2022 at 12:25 AM Srinandan Sridhar @.***>
wrote:

The problem was upx https://github.com/upx/upx. I'm using version 3.96,
which also happens to be the latest. I have removed upx for all version
(even though it worked for linux.
@ssvaidyanathan https://github.com/ssvaidyanathan confirmed it works
for him. Please try this binary
https://github.com/apigee/apigeecli/releases/tag/v1.113.5-beta. I may
have a new release v1.113 since this is impacting many users.
—
Reply to this email directly, view it on GitHub
https://github.com/apigee/apigeecli/issues/98#issuecomment-1274069045,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AD35LKVD362N5L26JDTOYM3WCTT2JANCNFSM6AAAAAARBTQNJ4
.
You are receiving this because you authored the thread.Message ID:
@.***>

This is fixed in v1.113


Inline styles and CSP

Since inline styles are used here:
https://github.com/Keyamoon/svgxuse/blob/master/svgxuse.js#L120-L123
svg.style.position = "absolute";
svg.style.width = 0;
svg.style.height = 0;
svg.style.overflow = "hidden";

one is forced to include style-src: unsafe-inline in CSP.
Would you be open to having a .css with a class for this specific case?

Suggested fix:
svgxuse.css
.svg-hide {
  overflow: hidden;
  position: absolute;
  width: 0;
  height: 0;
}

and change the referenced 4 lines to this:
svg.classList ? svg.classList.add('svg-hide') : svg.className += ' svg-hide';

I don't think so. That would add an additional HTTP request, and it would also make it harder to use for most people.

It's not an issue for me with a build script, but I perfectly understand your reasoning. If anyone comes across this issue, at least they have a simple solution :)
Thanks for the polyfill!


[Join] SkipRowsRange

Scope & Purpose
Implement support for LIMIT calls.
This PR introduces some code duplication. This will be fixed in a follow up PR.

@maierlars tests now green. Can be merged if you're happy with my test-split solution.


Unexpected and inconsistent behaviour with std::filesystem::path

I observed some really confusing and inconsistent behaviour when trying to deserialize a std::filesystem::path struct member
CXX compiler is MSVC 19.39.33523.0 (Visual Studio 2022 v17.9.5)
C++ Standard Library 14.39.33519 / Windows SDK 10.0.22621.0 on Windows 10.0.19045.
The following does not compile with this compiler error:
glaze-src\include\glaze\reflection\to_tuple.hpp(70,25): error C3448: the number of identifiers must match the number of array elements or members in a structured binding declaration
struct test_struct {
    uint32_t i {0};
    std::filesystem::path p {"."};
};
constexpr static auto json = R"({"i": 1, "p": "C:/123"})";

auto test = glz::read_json<test_struct>(json);

The next code variation does compile but does not work:
struct test_struct {
    std::filesystem::path p {"."};
};
constexpr static auto json = R"({"p": "C:/123"})";

auto test = glz::read_json<test_struct>(json);

JSON parse error (22): 1:2: unknown_key
   {"p": "C:\123"}
    ^

If I introduce a custom deserializer for std::filesystem::path as follows the code compiles but still does not deserialize the path correctly
namespace glz::detail {
template<> struct from_json<std::filesystem::path> {
    template <auto Opts>
    static void op(std::filesystem::path& path, auto&&... args) {
        std::string str;
        read<json>::op<Opts>(str, args...);
        path = std::filesystem::path {str};
    }
};
}

Only after I introduce an explicit glz::meta struct combined with the custom deserializer, it works as intended:
struct test_struct
{
    std::filesystem::path p {"."};
    struct glaze {
        static constexpr auto value = glz::object(&test_struct::p);
    };
};

Having the explicit glz::meta but without providing the custom deserializer, I get this compiler error:
glaze-src\include\glaze\json\read.hpp(974,34): error C2039: 'size': is not a member of 'std::filesystem::path'
Finally, wrapping the std::filesystem::path in a container like std::vector or std::optional it all compiles and works as intended while providing the custom deserializer but having no explicit glz::meta:
struct test_struct
{
    std::optional<std::filesystem::path> p {"."};
}

Thanks for bringing this up. I've never tested/used std::filesystem::path with Glaze, but it should definitely be supported. Thanks for providing a detailed analysis. I'll look into this more soon.

@dbraeckelmann, support has been added for std::filesystem::path. However, this support requires its use in glaze metadata (e.g. glz::meta) on GCC and MSVC. I could not get pure reflection to work in GCC and MSVC, which seem like bugs with aggregate initialization of structs with std::filesystem::path.


feat: limited number of confirmation skips

closes https://github.com/ErikBjare/gptme/issues/332

[!IMPORTANT]
Adds limited confirmation skips to ask_execute() using auto_skip_count in ask_execute.py.

Behavior:

Introduces auto_skip_count in ask_execute.py to limit the number of confirmation skips.
Modifies ask_execute() to decrement auto_skip_count when skipping confirmation.
Uses regex to parse "auto" command with optional skip count in ask_execute().

Misc:

Imports re module in ask_execute.py for regex operations.

This description was created by  for c035a16264fd8b20fa33132dc22028372362a680. It will automatically update as commits are pushed.

closes #332

Nice!

Forgot to [auto] as an option displayed in confirmation prompt. Guess it
remains secret?
On Thu, 19 Dec 2024, 15:03 Erik Bjäreholt, @.***> wrote:

Merged #346 https://github.com/ErikBjare/gptme/pull/346 into master.
—
Reply to this email directly, view it on GitHub
https://github.com/ErikBjare/gptme/pull/346#event-15716636740, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AO6ENNOYJ7ZUJ4VKOHSZJI32GKYYFAVCNFSM6AAAAABTYHLHXSVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJVG4YTMNRTGY3TIMA
.
You are receiving this because you authored the thread.Message ID:
@.***>

*add
On Thu, 19 Dec 2024, 15:08 Brayo Vuku, @.***> wrote:

Forgot to [auto] as an option displayed in confirmation prompt. Guess it
remains secret?
On Thu, 19 Dec 2024, 15:03 Erik Bjäreholt, @.***>
wrote:

Merged #346 https://github.com/ErikBjare/gptme/pull/346 into master.
—
Reply to this email directly, view it on GitHub
https://github.com/ErikBjare/gptme/pull/346#event-15716636740, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AO6ENNOYJ7ZUJ4VKOHSZJI32GKYYFAVCNFSM6AAAAABTYHLHXSVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJVG4YTMNRTGY3TIMA
.
You are receiving this because you authored the thread.Message ID:
@.***>


It is not possible to edit a database if it is not accessible

We have a database connection that is not accessible anymore. I want to remove it from sql lab view but since database is not accessible I'm getting error in the picture.
Expected results
Database should be saved successfully.
Actual results
Giving below error and doesn't save

How to reproduce the bug

Add any database
Make database inaccessible
Try to edit database sqllab settings (i.e. disable sql lab expose)

Environment
(please complete the following information):

superset version: 1.0.1
python version: 3.7

Checklist
Make sure to follow these steps before submitting your issue - thank you!

[x] I have checked the superset logs for python stacktraces and included it here as text if there are any.
[x] I have reproduced the issue with at least the latest released version of superset.
[x] I have checked the issue tracker for the same issue and I haven't found one similar.

Every DB connection you create is shown on SQLLab (if the user has permissions for it). On this case I think you actually want to remove the connection (delete the record from the db connection list)

No I don't want to remove database in case I may use it in future. Also this is not just about sqllab, database is not editable in anyway if it is inaccessible. I just gave sqllab situation  as an example.

Yes, I understand now. By design when an update is done, superset fetches the existing schemas from the db and updates the necessary security permissions. This behaviour can probably be improved if it's a problem to not be able to edit for example SQLLab database properties.
Can you explain in more detail, your use case for editing SQLLab properties for a database that is not accessible?

I just wanted to disable it from sqllab for more clear view. I think there is no need to fail because of a connection issue for every update action while there is a 'test connection' button there.

Closing this since it's been silent for two years, and sounds like it might be more of a feature request than a bug. Happy to revisit and reopen if that's warranted!


add TOKENIZERS_PARALLELISM environment variable after import HuggingF…

…aceEmbeddings
This can be fixed warning messages in #113
close #113

LGTM


How to capture a camera frame (still bitmap from video) ?

This question is similar to the one that was asked here: https://github.com/twilio/video-quickstart-android/issues/89 ... but for iOS.
Use case
While Twilio is capturing the video, we want the ability to take a bitmap frame from the video from time to time, so we can send the bitmap to our image processing engine, which works only on bitmaps.
Solution Approach using TVIVideoRenderer
extension TwilioCamera : TVIVideoRenderer {
    func renderFrame(_ frame: TVIVideoFrame) {
        // Very simple test to get Image from Video Buffer
        let size = CVImageBufferGetCleanRect(imageBuffer)

        // Fails with error: [CIImage initWithCVPixelBuffer:options:] failed
        // because its pixel format f420 is not supported.
        // Equates to kCVPixelFormatType_420YpCbCr8PlanarFullRange ('f420')
        // Need a YUL420p to ARGB converter here instead of this...
        let ciImage = CIImage(cvImageBuffer: imageBuffer, options: nil)
        ...
    }
...
}

In fact at the point where it does let ciImage = ... a converter from YUL420p to AGRB (or RGBA)  is required, as indicated in the documentation of renderFrame:

Your renderer is required to support frames in the format TVIPixelFormatYUV420PlanarFullRange. You should strongly reference (retain) the TVIVideoFrame before this call returns if you want to render it later. Frames are delivered to your renderer at or near their presentation timestamps.

The solution on the Android thread suggests to use OpenCV to perform that conversion. However, we do not want to bring OpenCV into our app just for the purpose of getting a converter which should basically be just one function.
Here are some more or less related links

https://stackoverflow.com/questions/25659671/how-to-convert-from-yuv-to-ciimage-for-ios
https://stackoverflow.com/questions/11267693/fastest-yuv420p-to-rgba-conversion-on-ios-using-the-cpu
https://stackoverflow.com/questions/14765892/how-to-convert-a-kcvpixelformattype-420ypcbcr8biplanarfullrange-buffer-to-yuv420
Libyuv: https://github.com/illuspas/libyuv-iOS
Neon: https://stackoverflow.com/questions/8451149/yuv-to-rgba-on-apple-a4-should-i-use-shaders-or-neon
Helpers: https://developer.apple.com/documentation/accelerate/vimage_transform

Questions...

Is the use of TVIVideoRenderer the only means of achieving this in Twilio Video ?
If so, does Twilio or anyone have a ready-made solution to hook in a YUV420p to ARGB converter so that the app can get a valid ciImage in the above example code ?
Did anyone do the same, and if so can an easy to integrate ready-made solution be provided ?

Hi @MBuLoft1,

While Twilio is capturing the video, we want the ability to take a bitmap frame from the video from time to time, so we can send the bitmap to our image processing engine, which works only on bitmaps.

If I understand your use case correctly, you want to create bitmaps from local video captured by TVICameraCapturer. If so, this is doable without writing very much code or performing the conversion yourself. Your TVIVideoRenderer just needs to opt into into receiving NV12 images - this is the planar video format which is widely supported by Apple frameworks. I've created a branch which demonstrates creating CIImages from the contents of TVICameraCapturer using TVIVideoRenderer.
https://github.com/ceaglest/video-quickstart-swift/tree/issue-142
As for remote video you would unfortunately need to do the conversion from I420 to NV12 (or ARGB) yourself before providing them to CIImage. For more discussion on this topic see: https://github.com/twilio/video-quickstart-swift/issues/123.
Best,
Chris Eagleston

Thank you. Added a note in #123 about going from I420 to BGRA.

Hopefully between this ticket, and #123 you've got the answer that you needed. We will do our best to release some sample code which demonstrates what was discussed in these two tickets.

Hi @ceaglest ,
I download this fork and tried running 'VideoQuickStart' on my device.
I get this exception stack trace in debugger and finally a crash with this error.
validateRenderPassDescriptor:842: failed assertion `Texture at depthAttachment has usage (0x01) which doesn't specify MTLTextureUsageRenderTarget (0x04)'
Main Thread Checker: UI API called on a background thread: -[UIApplication statusBarOrientation] PID: 982, TID: 336020, Thread name: (none), Queue name: com.twilio.cameracapturepipeline.session, QoS: 25 Backtrace: 4   TwilioVideo                         0x00000001010bdb70 +[UIApplication(TVIAdditions) tvi_statusBarOrientation] + 72 5   TwilioVideo                         0x0000000100c411f8 -[TVIUserInterfaceObserver observeWithDelegate:] + 48 6   TwilioVideo                         0x0000000100c3ada0 -[TVICameraCapturePipeline setupCaptureSessionWithConsumer:error:] + 1704 7   TwilioVideo                         0x0000000100c39ac0 __60-[TVICameraCapturePipeline startWithConsumer:configuration:]_block_invoke + 48 8   libdispatch.dylib                   0x0000000104a1e338 _dispatch_call_block_and_release + 24 9   libdispatch.dylib                   0x0000000104a1f730 _dispatch_client_callout + 16 10  libdispatch.dylib                   0x0000000104a26740 _dispatch_lane_serial_drain + 744 11  libdispatch.dylib                   0x0000000104a272e0 _dispatch_lane_invoke + 444 12  libdispatch.dylib                   0x0000000104a326c4 _dispatch_workloop_worker_thread + 1304 13  libsystem_pthread.dylib             0x00000001bd607b74 _pthread_wqthread + 272 14  libsystem_pthread.dylib             0x00000001bd60a740 start_wqthread + 8 2020-05-18 17:49:15.410563+0530 VideoQuickStart[982:336020] [reports] Main Thread Checker: UI API called on a background thread: -[UIApplication statusBarOrientation] PID: 982, TID: 336020, Thread name: (none), Queue name: com.twilio.cameracapturepipeline.session, QoS: 25 Backtrace: 4   TwilioVideo                         0x00000001010bdb70 +[UIApplication(TVIAdditions) tvi_statusBarOrientation] + 72 5   TwilioVideo                         0x0000000100c411f8 -[TVIUserInterfaceObserver observeWithDelegate:] + 48 6   TwilioVideo                         0x0000000100c3ada0 -[TVICameraCapturePipeline setupCaptureSessionWithConsumer:error:] + 1704 7   TwilioVideo                         0x0000000100c39ac0 __60-[TVICameraCapturePipeline startWithConsumer:configuration:]_block_invoke + 48 8   libdispatch.dylib                   0x0000000104a1e338 _dispatch_call_block_and_release + 24 9   libdispatch.dylib                   0x0000000104a1f730 _dispatch_client_callout + 16 10  libdispatch.dylib                   0x0000000104a26740 _dispatch_lane_serial_drain + 744 11  libdispatch.dylib                   0x0000000104a272e0 _dispatch_lane_invoke + 444 12  libdispatch.dylib                   0x0000000104a326c4 _dispatch_workloop_worker_thread + 1304 13  libsystem_pthread.dylib             0x00000001bd607b74 _pthread_wqthread + 272 14  libsystem_pthread.dylib             0x00000001bd60a740 start_wqthread + 8
However just before the crash, i can see 'renderFrame' being invoked and i can get ImageData as a CImage.
Regarding TVIVideoRenderer, when does 'renderFrame' get invoked?  What triggers renderFrame to be invoked?
Would appreciate any inputs.


boost connection disconnect fixes

fixing boost connection disconnects
also zrpcReq destroyed signal was fixed

Looks good, just need to have the ZrpcRequest destructor emit destroyed()