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

Do not require all extra params in salesforce hook #18929

Conversation

dstandish
Copy link
Contributor

This is a small fixup for the salesforce hook.

Previously, code used extras["blah"] or None instead of simply extras.get("blah"), which forces users to add each of these optional params to the connection extra.

In this PR we use extras.get, which means user only needs to supply the extra param if necessary.

Previously, code used `extras["blah"] or None` instead of simply `extras.get("blah")`.  This forces users to add each of these optional params to the connection extra.  Now, we use `extras.get` and you only need to supply the extra param if you need it.
@eladkal
Copy link
Contributor

eladkal commented Oct 13, 2021

see #18800

@josh-fell
Copy link
Contributor

With how the custom connection fields currently work, there will always be a key for each of the custom fields in Extra. Since each one of these Salesforce connection params have their own custom field in the connection form, if a value is not provided it defaults to "". Users aren't required to enter a value for each extra param.

In the simple-salesforce package, the logic that determines what authentication type to use are all similar in format to:

if all(arg is not None for arg in (username, password, security_token)):
    ...

Unfortunately there is no authentication_type that is passed to explicitly state the authentication a user would like to integrate with.

Because the empty string is not None, there are chances that the incorrect authentication type is chosen which was the choice for the slightly clunky extras["extra__salesforce__<conn-field>"] or None approach.

@josh-fell
Copy link
Contributor

All the authentication type logic in the Salesforce API starts here if you'd like to check it out.

@dstandish
Copy link
Contributor Author

dstandish commented Oct 13, 2021

@josh-fell if you use secrets backend then what you say about custom connection fields does not apply, and when you upgrade to this provider, your connection no longer works

@josh-fell
Copy link
Contributor

@josh-fell if you use secrets backend then what you say about customer connection fields does not apply, and when you upgrade to this provider, your connection no longer works

Ah I see. Valid point.

@dstandish
Copy link
Contributor Author

I see so maybe we need to do get or None instead of slice or None?

@josh-fell
Copy link
Contributor

I see so maybe we need to do get or None instead of slice or None?

Seems like the best approach.

@alexbegg
Copy link
Contributor

alexbegg commented Nov 10, 2021

I see so maybe we need to do get or None instead of slice or None?

@dstandish / @josh-fell
Doesn't get default to None? then why would this need a get or None?
Edit: Never mind, I just understood that if the key exists but it is an empty string, it should change to None in order to not break the built-in simple-salesforce authentication choosing method. Now I understand the need to get or None.

@alexbegg
Copy link
Contributor

My PR is open that handles the defaulting to None issue (and also updates the tests to test it is handled correctly: #19530

@eladkal
Copy link
Contributor

eladkal commented Nov 13, 2021

Closing as issue solved in #19530

@eladkal eladkal closed this Nov 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants