Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix app open when multiple app providers are present #2118

Merged
merged 3 commits into from
Oct 4, 2021
Merged

fix app open when multiple app providers are present #2118

merged 3 commits into from
Oct 4, 2021

Conversation

wkloucek
Copy link
Contributor

@wkloucek wkloucek commented Sep 29, 2021

Bugfix: Fix app open when multiple app providers are present

We've fixed the gateway behavior, that when multiple app providers are present, it always returned that we have duplicate names for app providers.
This was due the call to GetAllProviders() without any subsequent filtering by name. Now this filter mechanism is in place and the duplicate app providers error will only appear if a real duplicate is found.

Introduced in #2095

@glpatcern
Copy link
Member

I had a look at the code but there's something I don't understand, will discuss it with @ishank011 and @gmgigi96.

@wkloucek
Copy link
Contributor Author

wkloucek commented Oct 4, 2021

I had a look at the code but there's something I don't understand, will discuss it with @ishank011 and @gmgigi96.

Is there something I can help you?

@ishank011
Copy link
Contributor

@wkloucek looks good. Ideally filteredProviders should always have one element only but in case multiple apps are registered with the same name, should we raise an error?

@wkloucek
Copy link
Contributor Author

wkloucek commented Oct 4, 2021

@wkloucek looks good. Ideally filteredProviders should always have one element only but in case multiple apps are registered with the same name, should we raise an error?

That was already there (

// we should never arrive to the point of having more than one
// provider for the given "app" parameters sent by the user
return nil, errtypes.InternalError(fmt.Sprintf("gateway: user requested app %q and we provided %d applications", app, len(res.Providers)))
). Only the filter was missing

@ishank011
Copy link
Contributor

ishank011 commented Oct 4, 2021

Okay yes, my bad. Can you just move the code you added below the res.Status check please?

@ishank011
Copy link
Contributor

Also, sorry for the repeated reviews, but can you also remove the check if p.Name == app { inside the block where we check that there's only one provider? It's redundant

@wkloucek
Copy link
Contributor Author

wkloucek commented Oct 4, 2021

Okay yes, my bad. Can you just move the code you added below the res.Status check please?

changed 👍

Also, sorry for the repeated reviews, but can you also remove the check if p.Name == app { inside the block where we check that there's only one provider? It's redundant

That should stay... Image the user requesting an app that doesn't exist. The user would then get the only app existing, which is not the app, the user requested.

@ishank011
Copy link
Contributor

That should stay... Image the user requesting an app that doesn't exist. The user would then get the only app existing, which is not the app, the user requested.

But your code should take care of that, right?

@wkloucek
Copy link
Contributor Author

wkloucek commented Oct 4, 2021

That should stay... Image the user requesting an app that doesn't exist. The user would then get the only app existing, which is not the app, the user requested.

But your code should take care of that, right?

You're right, sorry...

Copy link
Contributor

@ishank011 ishank011 left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@ishank011 ishank011 merged commit 4b473fd into cs3org:master Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants