Add CollectionInfo in all Collections that have total_entries #14366

zachliu · 2021-02-22T20:26:00Z

Added a missing property in ConnectionCollection to avoid this error when using the generated Python client:

ApiAttributeError: ConnectionCollection has no attribute 'total_entries' at ['received_data']['total_entries']

See here:

airflow/airflow/api_connexion/schemas/connection_schema.py

Line 53 in 3a046fa

total_entries: int

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

boring-cyborg · 2021-02-22T20:26:04Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

kaxil · 2021-02-23T00:14:43Z

cc @ephraimbuddy

kaxil

Can you add a test please?

github-actions · 2021-02-23T00:15:36Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

ephraimbuddy · 2021-02-23T06:44:33Z

airflow/api_connexion/openapi/v1.yaml

@@ -1425,6 +1425,8 @@ components:
          type: array
          items:
            $ref: '#/components/schemas/ConnectionCollectionItem'
+        total_entries:
+          type: integer


While this will work, we have a CollectionInfo component. I will suggest we use it and expand to other Collection components that do not have total_entries.
For the ConnectionCollection, I would suggest this:

ConnectionCollection: type: object description: Connections allOf: - type: object properties: connections: type: array items: $ref: '#/components/schemas/ConnectionCollectionItem' - $ref: '#/components/schemas/CollectionInfo'

This is because the CollectionInfo component explains what total_entries is and since this change will affect many components, it's better we use the CollectionInfo component.
You can expand this change to DAGRunCollection, PoolCollection, VariableCollection etc

allOf is always a headache in openapi 3.0, but surprisingly this one works
my guess is because CollectionInfo is not nullable otherwise we are doomed 😂
i'll add the rest, thanks 👍

@ephraimbuddy done 🚀
i updated all collections for all available API endpoints except ExtraLinkCollection and TaskCollection which don't have total_entries

zachliu · 2021-02-23T15:24:40Z

Can you add a test please?

forgive my ignorance on this... since i didn't change the underlying API models, i thought there was no way to really test it until we generate clients using the specs 🤔
but if you meant the tests on the ConnectionCollection model itself, they already exist:

airflow/tests/api_connexion/schemas/test_connection_schema.py

Lines 120 to 141 in b995127

    
           instance = ConnectionCollection(connections=connections, total_entries=2) 
        
           deserialized_connections = connection_collection_schema.dump(instance) 
        
           assert deserialized_connections == { 
        
               'connections': [ 
        
                   { 
        
                       "connection_id": "mysql_default_1", 
        
                       "conn_type": "test-type", 
        
                       "host": None, 
        
                       "login": None, 
        
                       'schema': None, 
        
                       'port': None, 
        
                   }, 
        
                   { 
        
                       "connection_id": "mysql_default_2", 
        
                       "conn_type": "test-type2", 
        
                       "host": None, 
        
                       "login": None, 
        
                       'schema': None, 
        
                       'port': None, 
        
                   }, 
        
               ], 
        
               'total_entries': 2,

kaxil · 2021-02-23T16:40:05Z

Can you add a test please?

forgive my ignorance on this... since i didn't change the underlying API models, i thought there was no way to really test it until we generate clients using the specs 🤔
but if you meant the tests on the ConnectionCollection model itself, they already exist:

airflow/tests/api_connexion/schemas/test_connection_schema.py

Lines 120 to 141 in b995127

instance = ConnectionCollection(connections=connections, total_entries=2)

deserialized_connections = connection_collection_schema.dump(instance)

assert deserialized_connections == {

'connections': [

{

"connection_id": "mysql_default_1",

"conn_type": "test-type",

"host": None,

"login": None,

'schema': None,

'port': None,

},

{

"connection_id": "mysql_default_2",

"conn_type": "test-type2",

"host": None,

"login": None,

'schema': None,

'port': None,

},

],

'total_entries': 2,

Right, yeah that should suffice

zachliu · 2021-02-26T17:30:20Z

i had to fix a few other 🐛 to make these endpoints work in my python sdk, for example, several missing nullable items and some missing properties like running_slots in Pool
I'll create new PRs for them

ephraimbuddy · 2021-02-26T18:53:43Z

i had to fix a few other to make these endpoints work in my python sdk, for example, several missing nullable items and some missing properties like running_slots in Pool
I'll create new PRs for them

Let's fix only the collection info for this PR, You can make another PR for another one.
And since you have added collection info to the components, you have to remove it from the 200 responses of the affected endpoints where we also have collection info right now. E.g :

airflow/airflow/api_connexion/openapi/v1.yaml

Lines 270 to 271 in 11d03d2

    
           - $ref: '#/components/schemas/ConnectionCollection' 
        
           - $ref: '#/components/schemas/CollectionInfo'

You have to remove CollectionInfo here since it's already part of the other component

zachliu · 2021-02-26T19:12:38Z

i had to fix a few other to make these endpoints work in my python sdk, for example, several missing nullable items and some missing properties like running_slots in Pool
I'll create new PRs for them

Let's fix only the collection info for this PR, You can make another PR for another one.
And since you have added collection info to the components, you have to remove it from the 200 responses of the affected endpoints where we also have collection info right now. E.g :

airflow/airflow/api_connexion/openapi/v1.yaml

Lines 270 to 271 in 11d03d2

- $ref: '#/components/schemas/ConnectionCollection'

- $ref: '#/components/schemas/CollectionInfo'

You have to remove CollectionInfo here since it's already part of the other component

ha, i actually removed them long ago since they never worked with the openapi python generator
but @mik-laj said not to update them since other generator may need them. See here.

ephraimbuddy · 2021-02-26T19:18:05Z

i had to fix a few other to make these endpoints work in my python sdk, for example, several missing nullable items and some missing properties like running_slots in Pool
I'll create new PRs for them

Let's fix only the collection info for this PR, You can make another PR for another one.
And since you have added collection info to the components, you have to remove it from the 200 responses of the affected endpoints where we also have collection info right now. E.g :

airflow/airflow/api_connexion/openapi/v1.yaml

Lines 270 to 271 in 11d03d2

- $ref: '#/components/schemas/ConnectionCollection'

- $ref: '#/components/schemas/CollectionInfo'

You have to remove CollectionInfo here since it's already part of the other component

ha, i actually removed them long ago since they never worked with the openapi python generator
but someone here told me not to update them since other generator may need them. See here.

You have to remove it. The total entries is now inside the ConnectionCollection component. Without removing it here, there tests will not pass

zachliu · 2021-02-26T19:22:30Z

i had to fix a few other to make these endpoints work in my python sdk, for example, several missing nullable items and some missing properties like running_slots in Pool
I'll create new PRs for them

Let's fix only the collection info for this PR, You can make another PR for another one.
And since you have added collection info to the components, you have to remove it from the 200 responses of the affected endpoints where we also have collection info right now. E.g :

airflow/airflow/api_connexion/openapi/v1.yaml

Lines 270 to 271 in 11d03d2

- $ref: '#/components/schemas/ConnectionCollection'

- $ref: '#/components/schemas/CollectionInfo'

You have to remove CollectionInfo here since it's already part of the other component

ha, i actually removed them long ago since they never worked with the openapi python generator
but someone here told me not to update them since other generator may need them. See here.

You have to remove it. The total entries is now inside the ConnectionCollection component. Without removing it here, there tests will not pass

happy to 😁

zachliu · 2021-02-26T19:26:02Z

i had to fix a few other to make these endpoints work in my python sdk, for example, several missing nullable items and some missing properties like running_slots in Pool
I'll create new PRs for them

Let's fix only the collection info for this PR, You can make another PR for another one.
And since you have added collection info to the components, you have to remove it from the 200 responses of the affected endpoints where we also have collection info right now. E.g :

airflow/airflow/api_connexion/openapi/v1.yaml

Lines 270 to 271 in 11d03d2

- $ref: '#/components/schemas/ConnectionCollection'

- $ref: '#/components/schemas/CollectionInfo'

You have to remove CollectionInfo here since it's already part of the other component

ha, i actually removed them long ago since they never worked with the openapi python generator
but someone here told me not to update them since other generator may need them. See here.

You have to remove it. The total entries is now inside the ConnectionCollection component. Without removing it here, there tests will not pass

done 🚀

github-actions · 2021-02-26T19:53:01Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

github-actions · 2021-02-26T20:28:15Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

ephraimbuddy · 2021-02-27T23:05:04Z

airflow/api_connexion/openapi/v1.yaml

+       - type: object
+         properties:
+           connections:
+            type: array


You have linting errors in ConnectionCollection. Can you install pre-commit so you can find issues locally before committing. https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#pre-commit-hooks.

This should work:

ConnectionCollection: type: object description: Collection of connections. allOf: - type: object properties: connections: type: array items: $ref: '#/components/schemas/ConnectionCollectionItem' - $ref: '#/components/schemas/CollectionInfo'

If you accept, I can push a fix for you but first try and see if you are able to resolve it. Thanks

done 🚀
sorry about that, planned to do it after the weekend 😁

github-actions · 2021-03-01T23:15:57Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

boring-cyborg · 2021-03-01T23:31:17Z

Awesome work, congrats on your first merged pull request!

missing total_entries in ConnectionCollection

021da40

zachliu requested review from kaxil and mik-laj as code owners February 22, 2021 20:26

boring-cyborg bot added the area:API Airflow's REST/HTTP API label Feb 22, 2021

kaxil approved these changes Feb 23, 2021

View reviewed changes

github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Feb 23, 2021

ephraimbuddy requested changes Feb 23, 2021

View reviewed changes

use CollectionInfo for all API collection endpoints

19e557d

zachliu changed the title ~~Add total_entries property in ConnectionCollection~~ Add CollectionInfo in all Collections that have total_entries Feb 26, 2021

github-actions bot removed the okay to merge It's ok to merge this PR as it does not require more tests label Feb 26, 2021

remove CollectionInfo from 200 responses

e8e3702

typo 💩

f5b826d

ephraimbuddy reviewed Feb 27, 2021

View reviewed changes

typo again 💩

1065f56

ephraimbuddy approved these changes Mar 1, 2021

View reviewed changes

github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Mar 1, 2021

ephraimbuddy merged commit 1f5b764 into apache:master Mar 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CollectionInfo in all Collections that have total_entries #14366

Add CollectionInfo in all Collections that have total_entries #14366

zachliu commented Feb 22, 2021

boring-cyborg bot commented Feb 22, 2021

kaxil commented Feb 23, 2021

kaxil left a comment

github-actions bot commented Feb 23, 2021

ephraimbuddy Feb 23, 2021 •

edited

zachliu Feb 23, 2021

zachliu Feb 26, 2021

zachliu commented Feb 23, 2021

kaxil commented Feb 23, 2021 •

edited

zachliu commented Feb 26, 2021

ephraimbuddy commented Feb 26, 2021

zachliu commented Feb 26, 2021 •

edited

ephraimbuddy commented Feb 26, 2021

zachliu commented Feb 26, 2021

zachliu commented Feb 26, 2021

github-actions bot commented Feb 26, 2021

github-actions bot commented Feb 26, 2021

ephraimbuddy Feb 27, 2021

zachliu Mar 1, 2021

github-actions bot commented Mar 1, 2021

boring-cyborg bot commented Mar 1, 2021

Add CollectionInfo in all Collections that have total_entries #14366

Add CollectionInfo in all Collections that have total_entries #14366

Conversation

zachliu commented Feb 22, 2021

boring-cyborg bot commented Feb 22, 2021

kaxil commented Feb 23, 2021

kaxil left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 23, 2021

ephraimbuddy Feb 23, 2021 • edited

Choose a reason for hiding this comment

zachliu Feb 23, 2021

Choose a reason for hiding this comment

zachliu Feb 26, 2021

Choose a reason for hiding this comment

zachliu commented Feb 23, 2021

kaxil commented Feb 23, 2021 • edited

zachliu commented Feb 26, 2021

ephraimbuddy commented Feb 26, 2021

zachliu commented Feb 26, 2021 • edited

ephraimbuddy commented Feb 26, 2021

zachliu commented Feb 26, 2021

zachliu commented Feb 26, 2021

github-actions bot commented Feb 26, 2021

github-actions bot commented Feb 26, 2021

ephraimbuddy Feb 27, 2021

Choose a reason for hiding this comment

zachliu Mar 1, 2021

Choose a reason for hiding this comment

github-actions bot commented Mar 1, 2021

boring-cyborg bot commented Mar 1, 2021

ephraimbuddy Feb 23, 2021 •

edited

kaxil commented Feb 23, 2021 •

edited

zachliu commented Feb 26, 2021 •

edited