feat(gsheets2): added hidden properties #208
Conversation
testinnplayin
added
enhancement
Codecov Report
@@ Coverage Diff @@
## master #208 +/- ##
==========================================
+ Coverage 97.74% 97.75% +0.01%
==========================================
Files 43 43
Lines 2220 2231 +11
==========================================
+ Hits 2170 2181 +11
Misses 50 50
Continue to review full report at Codecov.
|
testinnplayin
force-pushed
the
f/hide-certain-properties
branch
from
df53a59 to
395e18a
Compare
testinnplayin
force-pushed
the
f/hide-certain-properties
branch
from
395e18a to
1d01379
Compare
testinnplayin
force-pushed
the
f/hide-certain-properties
branch
from
1d01379 to
25ce76a
Compare
There was a problem hiding this comment.
Thanks for all this work and research you put it in.
However, reading the code, I feel weird about the hidden_properties method.
First : it contains only a token for now, so maybe a generic name is not appropriate to make following readers understand what this is about. IMO this name would have make sense if this was in the base class ToucanConnectors when we have no clue what will be inside.
Second : I have suggestion to ease the API of the connector. Since what the connector needs is to retrieve an access_token using the auth_flow_id, maybe it's easier to just pass to a connector the method that does this, and then the connector will be able use it when needed. What do you think about this ?
|
|
||
| The `secrets` dictionary contains the `access_token` and a `refresh_token` (if there is one). Though `secrets` is optional during the initial creation of the connector, it is necessary for when the user wants to make requests to the connector. If there is no `access_token`, an Exception is thrown. | ||
| The `hidden_properties` propertie contains the `access_token` and a `refresh_token` (if there is one) in a class called `GoogleSheets2HiddenProperties`. Though `hidden_properties` is optional during the initial creation of the connector, it is necessary for when the user wants to make requests to the connector. If there is no `access_token`, an Exception is thrown. This is a dynamic *non-flag* property. |
There was a problem hiding this comment.
| The `hidden_properties` propertie contains the `access_token` and a `refresh_token` (if there is one) in a class called `GoogleSheets2HiddenProperties`. Though `hidden_properties` is optional during the initial creation of the connector, it is necessary for when the user wants to make requests to the connector. If there is no `access_token`, an Exception is thrown. This is a dynamic *non-flag* property. | |
| The `hidden_properties` property contains the `access_token` and a `refresh_token` (if there is one) in a class called `GoogleSheets2HiddenProperties`. Though `hidden_properties` is optional during the initial creation of the connector, it is necessary for when the user wants to make requests using the connector. If there is no `access_token`, an Exception is thrown. This is a dynamic property (not a static one). |
|
|
||
| The `baseroute` is fixed and is 'https://sheets.googleapis.com/v4/spreadsheets/'. | ||
| The `_baseroute` is fixed and is 'https://sheets.googleapis.com/v4/spreadsheets/'. It too is hidden from being rendered in the front. It is a static *non-flag* |
There was a problem hiding this comment.
| The `_baseroute` is fixed and is 'https://sheets.googleapis.com/v4/spreadsheets/'. It too is hidden from being rendered in the front. It is a static *non-flag* | |
| The `_baseroute` is fixed and is 'https://sheets.googleapis.com/v4/spreadsheets/'. It too is hidden from being rendered in the front. It is a static property. |
Or class property (something that doesn't change with the instance)
|
|
||
| The `auth_flow` property marks this as being a connector that uses the connector_oauth_manager for the oauth dance. | ||
| The `_auth_flow` property marks this as being a connector that uses the connector_oauth_manager for the oauth dance. It is hidden from being rendered in the front but appears in the front as a flag for initiating the oauth dance. It is a static *flag*. |
There was a problem hiding this comment.
| The `_auth_flow` property marks this as being a connector that uses the connector_oauth_manager for the oauth dance. It is hidden from being rendered in the front but appears in the front as a flag for initiating the oauth dance. It is a static *flag*. | |
| The `_auth_flow` property marks this as being a connector that uses the connector_oauth_manager for the oauth dance. It is hidden from being rendered in the front but appears in the front as a flag for initiating the oauth dance. It is a static *property*. |
I prefer we use the term property, which has a clear meaning in Python.
Flag is less clear : it suggest a boolean, a feature-flag, and many other things.
| @@ -54,20 +57,41 @@ def get_form(cls, connector: 'GoogleSheets2Connector', current_config): | |||
| Secrets = Dict[str, Any] | |||
|
|
|||
|
|
|||
| class GoogleSheets2HiddenProperties(BaseModel): | |||
| """ | |||
| This class contains dynamic hidden properties for Google Sheets 2 that are not flags | |||
There was a problem hiding this comment.
| This class contains dynamic hidden properties for Google Sheets 2 that are not flags | |
| This class contains dynamic hidden properties for Google Sheets 2 that are not configured directly by a user, like tokens retrieved via OAuth2 |
| auth_flow_id: Optional[str] | ||
|
|
||
| # The following should be hidden properties | ||
| # flagging variable; required for initiating the oauth dance |
| # static variable | ||
| _baseroute = 'https://sheets.googleapis.com/v4/spreadsheets/' |
There was a problem hiding this comment.
Could we make that a real class property by the way ? That will save some db space and prevent to send it to the front-end
| @@ -54,20 +57,41 @@ def get_form(cls, connector: 'GoogleSheets2Connector', current_config): | |||
| Secrets = Dict[str, Any] | |||
|
|
|||
|
|
|||
| class GoogleSheets2HiddenProperties(BaseModel): | |||
There was a problem hiding this comment.
I don't really see the value of using a Pydantic BaseModel here. Their usage is to validate/enforce a model, when some data comes from an uncertain source, like a form filled by someone, or an unknown database without a schema.
I wonder if a very simple dataclass would be enough here.
| auth_flow_id: Optional[str] | ||
|
|
||
| # The following should be hidden properties | ||
| # flagging variable; required for initiating the oauth dance | ||
| _auth_flow = 'oauth2' |
There was a problem hiding this comment.
I took a few minutes to look at Pydantic's docs and saw the keep_untouched config option.
https://pydantic-docs.helpmanual.io/usage/model_config/
Is that worth to try ?
|
I put in generic terms because I'm thinking of the future of the connectors. We could do similar things with other connectors if we wanted to go this route. If we have a retry policy in place for the connector, we might need the refresh token in secrets as well and not just the access token. I'm not sure about your second point sorry. |
Change Summary
We added a class to hide certain properties from the user. These can be divided into two main categories: flags and non-flags.
Flags are properties that are required by the front to do something. In this case
auth_flow_idis for initiating the oauth dance.Non-flags are everything else. Non-flags can be divided into two categories: static and dynamic. Static non-flags are kept on the connector itself and have a '_' at the front of their name. Dynamic ones are kept on the new class. The reason why we put the static non-flags is so that we don't have to return them if we don't want to when we fetch hidden properties from the class that contains them.
On the front-end, we will eventually want to hide all properties that are stored in 'hidden_properties' on the connector. Those properties that begin with a '_' are hidden automatically.
Dynamic properties cannot be hidden with a '_' in front of their variable name because we can't set them afterwards.
Related issue number
#206
Checklist