You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/06/12 23:35:49 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #9266: [WIP]add crud endpoint for connections
ephraimbuddy opened a new pull request #9266:
URL: https://github.com/apache/airflow/pull/9266
---
Closes: #9108
Make sure to mark the boxes below before creating PR: [x]
- [ ] Description above provides context of the change
- [ ] Unit tests coverage for changes (not needed for documentation changes)
- [ ] Target Github ISSUE in description if exists
- [ ] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [ ] Relevant documentation is updated including usage instructions.
- [ ] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9266: add crud endpoint for connections
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r439869721
##########
File path: airflow/api_connexion/endpoints/connection_endpoint.py
##########
@@ -47,31 +55,53 @@ def get_connection(connection_id, session):
@provide_session
-def get_connections(session):
+def get_connections(session, limit, offset=0):
"""
Get all connection entries
"""
- offset = request.args.get(parameters.page_offset, 0)
- limit = min(int(request.args.get(parameters.page_limit, 100)), 100)
-
+ total_entries = session.query(func.count(Connection.id)).scalar()
query = session.query(Connection)
- total_entries = query.count()
- query = query.offset(offset).limit(limit)
-
- connections = query.all()
+ connections = query.offset(offset).limit(limit).all()
return connection_collection_schema.dump(ConnectionCollection(connections=connections,
total_entries=total_entries))
-def patch_connection():
+@provide_session
+def patch_connection(connection_id, session, update_mask=None):
"""
Update a connection entry
"""
- raise NotImplementedError("Not implemented yet.")
+ body = request.json
+ connection = session.query(Connection).filter_by(conn_id=connection_id).first()
+ if connection is None:
+ raise NotFound("Connection not found")
+ if update_mask:
+ for field in update_mask:
+ setattr(connection, field, body[field])
+ else:
+ for key in body:
+ setattr(connection, key, body[key])
+ session.add(connection)
+ session.commit()
+ return connection_schema.dump(connection)
-def post_connection():
+@provide_session
+def post_connection(session):
"""
Create connection entry
"""
- raise NotImplementedError("Not implemented yet.")
+ body = request.json
+
+ # connexion handles 404, no need for try/except
+ result = connection_schema.load(body)
+ data = result.data
+ conn_id = data['conn_id']
Review comment:
```suggestion
conn_id = data['connection_id']
```
Is there a mistake here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #9266: [WIP]add crud endpoint for connections
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r439717022
##########
File path: tests/api_connexion/endpoints/test_connection_endpoint.py
##########
@@ -207,7 +230,32 @@ def test_should_response_200(self):
class TestPostConnection(TestConnectionEndpoint):
- @unittest.skip("Not implemented yet")
- def test_should_response_200(self):
- response = self.client.post("/api/v1/connections/")
+
+ @provide_session
+ def test_post_should_response_200(self, session):
+ payload = {
+ "connection_id": "test-connection-id",
+ "conn_type": 'test_type'
+ }
+ response = self.client.post("/api/v1/connections", json=payload)
assert response.status_code == 200
+ connection = session.query(Connection).all()
+ assert len(connection) == 1
+ self.assertEqual(connection[0].conn_id, 'test-connection-id')
+
+ def test_post_should_response_400_for_invalid_payload(self):
+ payload = {
+ "connection_id": "test-connection-id",
+ } # conn_type missing
+ response = self.client.post("/api/v1/connections", json=payload)
+ assert response.status_code == 400
+ self.assertEqual(response.json,
+ {'detail': "'conn_type' is a required property",
+ 'status': 400,
+ 'title': 'Bad Request',
+ 'type': 'about:blank'}
+ )
+
+ @unittest.skip('not implemented yet')
+ def test_delete_should_response_409_already_exist(self):
Review comment:
```suggestion
def test_post_should_response_409_already_exist(self):
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #9266: Add CRUD endpoint for connections
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r439981380
##########
File path: airflow/api_connexion/endpoints/connection_endpoint.py
##########
@@ -47,31 +55,53 @@ def get_connection(connection_id, session):
@provide_session
-def get_connections(session):
+def get_connections(session, limit, offset=0):
"""
Get all connection entries
"""
- offset = request.args.get(parameters.page_offset, 0)
- limit = min(int(request.args.get(parameters.page_limit, 100)), 100)
-
+ total_entries = session.query(func.count(Connection.id)).scalar()
query = session.query(Connection)
- total_entries = query.count()
- query = query.offset(offset).limit(limit)
-
- connections = query.all()
+ connections = query.offset(offset).limit(limit).all()
return connection_collection_schema.dump(ConnectionCollection(connections=connections,
total_entries=total_entries))
-def patch_connection():
+@provide_session
+def patch_connection(connection_id, session, update_mask=None):
"""
Update a connection entry
"""
- raise NotImplementedError("Not implemented yet.")
+ body = request.json
+ connection = session.query(Connection).filter_by(conn_id=connection_id).first()
+ if connection is None:
+ raise NotFound("Connection not found")
+ if update_mask:
Review comment:
Also check this, https://github.com/marshmallow-code/marshmallow-sqlalchemy/issues/33#issuecomment-147008000
For updatable fields, looks like we would need it in dagruns update
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #9266: add crud endpoint for connections
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r439870308
##########
File path: airflow/api_connexion/endpoints/connection_endpoint.py
##########
@@ -47,31 +55,53 @@ def get_connection(connection_id, session):
@provide_session
-def get_connections(session):
+def get_connections(session, limit, offset=0):
"""
Get all connection entries
"""
- offset = request.args.get(parameters.page_offset, 0)
- limit = min(int(request.args.get(parameters.page_limit, 100)), 100)
-
+ total_entries = session.query(func.count(Connection.id)).scalar()
query = session.query(Connection)
- total_entries = query.count()
- query = query.offset(offset).limit(limit)
-
- connections = query.all()
+ connections = query.offset(offset).limit(limit).all()
return connection_collection_schema.dump(ConnectionCollection(connections=connections,
total_entries=total_entries))
-def patch_connection():
+@provide_session
+def patch_connection(connection_id, session, update_mask=None):
"""
Update a connection entry
"""
- raise NotImplementedError("Not implemented yet.")
+ body = request.json
+ connection = session.query(Connection).filter_by(conn_id=connection_id).first()
+ if connection is None:
+ raise NotFound("Connection not found")
+ if update_mask:
+ for field in update_mask:
+ setattr(connection, field, body[field])
+ else:
+ for key in body:
+ setattr(connection, key, body[key])
+ session.add(connection)
+ session.commit()
+ return connection_schema.dump(connection)
-def post_connection():
+@provide_session
+def post_connection(session):
"""
Create connection entry
"""
- raise NotImplementedError("Not implemented yet.")
+ body = request.json
+
+ # connexion handles 404, no need for try/except
+ result = connection_schema.load(body)
+ data = result.data
+ conn_id = data['conn_id']
Review comment:
No. After passing through the` connection_schema`, it becomes `conn_id`. The dumping and loading comes from `connection_id`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9266: add crud endpoint for connections
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r439869565
##########
File path: airflow/api_connexion/endpoints/connection_endpoint.py
##########
@@ -47,31 +55,53 @@ def get_connection(connection_id, session):
@provide_session
-def get_connections(session):
+def get_connections(session, limit, offset=0):
"""
Get all connection entries
"""
- offset = request.args.get(parameters.page_offset, 0)
- limit = min(int(request.args.get(parameters.page_limit, 100)), 100)
-
+ total_entries = session.query(func.count(Connection.id)).scalar()
query = session.query(Connection)
- total_entries = query.count()
- query = query.offset(offset).limit(limit)
-
- connections = query.all()
+ connections = query.offset(offset).limit(limit).all()
return connection_collection_schema.dump(ConnectionCollection(connections=connections,
total_entries=total_entries))
-def patch_connection():
+@provide_session
+def patch_connection(connection_id, session, update_mask=None):
"""
Update a connection entry
"""
- raise NotImplementedError("Not implemented yet.")
+ body = request.json
+ connection = session.query(Connection).filter_by(conn_id=connection_id).first()
+ if connection is None:
+ raise NotFound("Connection not found")
+ if update_mask:
Review comment:
Can you also add validation for the mask? The mask should not contain fields that are on the allowed list. In particular, you should not be able to save any data to the _password field. This is a very common mistake, which leads to CATastrophe.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9266: Add CRUD endpoint for connections
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r443196939
##########
File path: tests/api_connexion/endpoints/test_connection_endpoint.py
##########
@@ -200,14 +229,215 @@ def _create_connections(self, count):
class TestPatchConnection(TestConnectionEndpoint):
- @unittest.skip("Not implemented yet")
- def test_should_response_200(self):
- response = self.client.patch("/api/v1/connections/1")
+
+ @provide_session
+ def test_patch_should_response_200(self, session):
+ self._create_connection(session)
+ payload = {
+ "connection_id": "test-connection-id",
+ "conn_type": 'test_type',
+ "extra": "{'key': 'var'}"
+ }
+ response = self.client.patch("/api/v1/connections/test-connection-id",
+ json=payload)
+ assert response.status_code == 200
+
+ @provide_session
+ def test_patch_should_response_200_with_update_mask(self, session):
+ self._create_connection(session)
+ test_connection = "test-connection-id"
+ payload = {
+ "connection_id": test_connection,
+ "conn_type": 'test_type_2',
+ "extra": "{'key': 'var'}",
+ 'login': "login",
+ "port": 80,
+ }
+ response = self.client.patch(
+ "/api/v1/connections/test-connection-id?update_mask=port,login",
+ json=payload
+ )
assert response.status_code == 200
+ connection = session.query(Connection).filter_by(conn_id=test_connection).first()
+ self.assertEqual(connection.password, None)
+ self.assertEqual(
+ response.json,
+ {
+ "connection_id": test_connection, # not updated
+ "conn_type": 'test_type', # Not updated
+ "extra": None, # Not updated
+ 'login': "login", # updated
+ "port": 80, # updated
+ "schema": None,
+ "host": None
+
+ }
+ )
+
+ @parameterized.expand(
+ [
+ (
+ {
+ "connection_id": 'test-connection-id',
Review comment:
Can you add a test that checks the request without the connection_id key?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9266: Add CRUD endpoint for connections
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r442901683
##########
File path: tests/api_connexion/endpoints/test_connection_endpoint.py
##########
@@ -200,14 +229,143 @@ def _create_connections(self, count):
class TestPatchConnection(TestConnectionEndpoint):
- @unittest.skip("Not implemented yet")
- def test_should_response_200(self):
- response = self.client.patch("/api/v1/connections/1")
+
+ @provide_session
+ def test_patch_should_response_200(self, session):
+ self._create_connection(session)
+ payload = {
+ "connection_id": "test-connection-id",
+ "conn_type": 'test_type',
+ "extra": "{'key': 'var'}"
+ }
+ response = self.client.patch("/api/v1/connections/test-connection-id",
+ json=payload)
+ assert response.status_code == 200
+
+ @provide_session
+ def test_patch_should_response_200_with_update_mask(self, session):
+ self._create_connection(session)
+ test_connection = "test-connection"
+ payload = {
+ "connection_id": test_connection,
Review comment:
It's a immutable fields.
```
When a service receives an immutable field in an update request (or similar), even if included in the update mask, the service should ignore the field if the value matches, but should error with INVALID_ARGUMENT if a change is requested.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #9266: add crud endpoint for connections
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r439868448
##########
File path: airflow/api_connexion/endpoints/connection_endpoint.py
##########
@@ -47,31 +55,53 @@ def get_connection(connection_id, session):
@provide_session
-def get_connections(session):
+def get_connections(session, limit, offset=0):
"""
Get all connection entries
"""
- offset = request.args.get(parameters.page_offset, 0)
- limit = min(int(request.args.get(parameters.page_limit, 100)), 100)
-
+ total_entries = session.query(func.count(Connection.id)).scalar()
query = session.query(Connection)
- total_entries = query.count()
- query = query.offset(offset).limit(limit)
-
- connections = query.all()
+ connections = query.offset(offset).limit(limit).all()
return connection_collection_schema.dump(ConnectionCollection(connections=connections,
total_entries=total_entries))
-def patch_connection():
+@provide_session
+def patch_connection(connection_id, session, update_mask=None):
"""
Update a connection entry
"""
- raise NotImplementedError("Not implemented yet.")
+ body = request.json
+ connection = session.query(Connection).filter_by(conn_id=connection_id).first()
+ if connection is None:
+ raise NotFound("Connection not found")
+ if update_mask:
+ for field in update_mask:
+ setattr(connection, field, body[field])
+ else:
+ for key in body:
+ setattr(connection, key, body[key])
+ session.add(connection)
+ session.commit()
+ return connection_schema.dump(connection)
-def post_connection():
+@provide_session
+def post_connection(session):
"""
Create connection entry
"""
- raise NotImplementedError("Not implemented yet.")
+ body = request.json
+
+ # connexion handles 404, no need for try/except
Review comment:
```suggestion
# connexion handles 400, no need for try/except
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #9266: Add CRUD endpoint for connections
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r443095754
##########
File path: tests/api_connexion/endpoints/test_connection_endpoint.py
##########
@@ -200,14 +229,143 @@ def _create_connections(self, count):
class TestPatchConnection(TestConnectionEndpoint):
- @unittest.skip("Not implemented yet")
- def test_should_response_200(self):
- response = self.client.patch("/api/v1/connections/1")
+
+ @provide_session
+ def test_patch_should_response_200(self, session):
+ self._create_connection(session)
+ payload = {
+ "connection_id": "test-connection-id",
+ "conn_type": 'test_type',
+ "extra": "{'key': 'var'}"
+ }
+ response = self.client.patch("/api/v1/connections/test-connection-id",
+ json=payload)
+ assert response.status_code == 200
+
+ @provide_session
+ def test_patch_should_response_200_with_update_mask(self, session):
+ self._create_connection(session)
+ test_connection = "test-connection"
+ payload = {
+ "connection_id": test_connection,
+ "conn_type": 'test_type_2',
+ "extra": "{'key': 'var'}",
+ 'login': "login",
+ "port": 80,
+ "_password": 'password',
+ 'nan': 'asdf'
+ }
+ response = self.client.patch(
+ "/api/v1/connections/test-connection-id?update_mask=port,login, connection_id, _password",
Review comment:
Fixed https://github.com/apache/airflow/pull/9266/commits/4eae4f6c53f595afae32d73a5f8544b7cfc60d57
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9266: Add CRUD endpoint for connections
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r442902437
##########
File path: tests/api_connexion/endpoints/test_connection_endpoint.py
##########
@@ -200,14 +229,143 @@ def _create_connections(self, count):
class TestPatchConnection(TestConnectionEndpoint):
- @unittest.skip("Not implemented yet")
- def test_should_response_200(self):
- response = self.client.patch("/api/v1/connections/1")
+
+ @provide_session
+ def test_patch_should_response_200(self, session):
+ self._create_connection(session)
+ payload = {
+ "connection_id": "test-connection-id",
+ "conn_type": 'test_type',
+ "extra": "{'key': 'var'}"
+ }
+ response = self.client.patch("/api/v1/connections/test-connection-id",
+ json=payload)
+ assert response.status_code == 200
+
+ @provide_session
+ def test_patch_should_response_200_with_update_mask(self, session):
+ self._create_connection(session)
+ test_connection = "test-connection"
+ payload = {
+ "connection_id": test_connection,
+ "conn_type": 'test_type_2',
+ "extra": "{'key': 'var'}",
+ 'login': "login",
+ "port": 80,
+ "_password": 'password',
+ 'nan': 'asdf'
+ }
+ response = self.client.patch(
+ "/api/v1/connections/test-connection-id?update_mask=port,login, connection_id, _password",
Review comment:
Unsupported fields in the mask should result in an INVALID_ARGUMENT exception. This avoids typos.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9266: add crud endpoint for connections
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r439878711
##########
File path: airflow/api_connexion/endpoints/connection_endpoint.py
##########
@@ -47,31 +55,53 @@ def get_connection(connection_id, session):
@provide_session
-def get_connections(session):
+def get_connections(session, limit, offset=0):
"""
Get all connection entries
"""
- offset = request.args.get(parameters.page_offset, 0)
- limit = min(int(request.args.get(parameters.page_limit, 100)), 100)
-
+ total_entries = session.query(func.count(Connection.id)).scalar()
query = session.query(Connection)
- total_entries = query.count()
- query = query.offset(offset).limit(limit)
-
- connections = query.all()
+ connections = query.offset(offset).limit(limit).all()
return connection_collection_schema.dump(ConnectionCollection(connections=connections,
total_entries=total_entries))
-def patch_connection():
+@provide_session
+def patch_connection(connection_id, session, update_mask=None):
"""
Update a connection entry
"""
- raise NotImplementedError("Not implemented yet.")
+ body = request.json
+ connection = session.query(Connection).filter_by(conn_id=connection_id).first()
+ if connection is None:
+ raise NotFound("Connection not found")
+ if update_mask:
Review comment:
Ah. Probably I was wrong, but I will still have to check how connexion behaves when it sees additional fields in the request from the user. It seems to me that it could be set in the connexion configuration, but I'm not sure. I will come back to this because it will take me a moment to check.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #9266: Add CRUD endpoint for connections
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r443225165
##########
File path: tests/api_connexion/endpoints/test_connection_endpoint.py
##########
@@ -200,14 +229,215 @@ def _create_connections(self, count):
class TestPatchConnection(TestConnectionEndpoint):
- @unittest.skip("Not implemented yet")
- def test_should_response_200(self):
- response = self.client.patch("/api/v1/connections/1")
+
+ @provide_session
+ def test_patch_should_response_200(self, session):
+ self._create_connection(session)
+ payload = {
+ "connection_id": "test-connection-id",
+ "conn_type": 'test_type',
+ "extra": "{'key': 'var'}"
+ }
+ response = self.client.patch("/api/v1/connections/test-connection-id",
+ json=payload)
+ assert response.status_code == 200
+
+ @provide_session
+ def test_patch_should_response_200_with_update_mask(self, session):
+ self._create_connection(session)
+ test_connection = "test-connection-id"
+ payload = {
+ "connection_id": test_connection,
+ "conn_type": 'test_type_2',
+ "extra": "{'key': 'var'}",
+ 'login': "login",
+ "port": 80,
+ }
+ response = self.client.patch(
+ "/api/v1/connections/test-connection-id?update_mask=port,login",
+ json=payload
+ )
assert response.status_code == 200
+ connection = session.query(Connection).filter_by(conn_id=test_connection).first()
+ self.assertEqual(connection.password, None)
+ self.assertEqual(
+ response.json,
+ {
+ "connection_id": test_connection, # not updated
+ "conn_type": 'test_type', # Not updated
+ "extra": None, # Not updated
+ 'login': "login", # updated
+ "port": 80, # updated
+ "schema": None,
+ "host": None
+
+ }
+ )
+
+ @parameterized.expand(
+ [
+ (
+ {
+ "connection_id": 'test-connection-id',
Review comment:
Fixed.https://github.com/apache/airflow/pull/9266/commits/1a4efd35d37bae28735af470d7846ff3b0bce6f6.
It happened that the required fields I added earlier in spec was affecting the update. I have removed it. It would have been fine in the spec to have the required field if there's a separate schema for updating. But with manual validation, everything seems fine. Please check
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #9266: Add CRUD endpoint for connections
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r443095821
##########
File path: tests/api_connexion/endpoints/test_connection_endpoint.py
##########
@@ -200,14 +229,143 @@ def _create_connections(self, count):
class TestPatchConnection(TestConnectionEndpoint):
- @unittest.skip("Not implemented yet")
- def test_should_response_200(self):
- response = self.client.patch("/api/v1/connections/1")
+
+ @provide_session
+ def test_patch_should_response_200(self, session):
+ self._create_connection(session)
+ payload = {
+ "connection_id": "test-connection-id",
+ "conn_type": 'test_type',
+ "extra": "{'key': 'var'}"
+ }
+ response = self.client.patch("/api/v1/connections/test-connection-id",
+ json=payload)
+ assert response.status_code == 200
+
+ @provide_session
+ def test_patch_should_response_200_with_update_mask(self, session):
+ self._create_connection(session)
+ test_connection = "test-connection"
+ payload = {
+ "connection_id": test_connection,
Review comment:
Fixed https://github.com/apache/airflow/pull/9266/commits/4eae4f6c53f595afae32d73a5f8544b7cfc60d57
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #9266: add crud endpoint for connections
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r439981380
##########
File path: airflow/api_connexion/endpoints/connection_endpoint.py
##########
@@ -47,31 +55,53 @@ def get_connection(connection_id, session):
@provide_session
-def get_connections(session):
+def get_connections(session, limit, offset=0):
"""
Get all connection entries
"""
- offset = request.args.get(parameters.page_offset, 0)
- limit = min(int(request.args.get(parameters.page_limit, 100)), 100)
-
+ total_entries = session.query(func.count(Connection.id)).scalar()
query = session.query(Connection)
- total_entries = query.count()
- query = query.offset(offset).limit(limit)
-
- connections = query.all()
+ connections = query.offset(offset).limit(limit).all()
return connection_collection_schema.dump(ConnectionCollection(connections=connections,
total_entries=total_entries))
-def patch_connection():
+@provide_session
+def patch_connection(connection_id, session, update_mask=None):
"""
Update a connection entry
"""
- raise NotImplementedError("Not implemented yet.")
+ body = request.json
+ connection = session.query(Connection).filter_by(conn_id=connection_id).first()
+ if connection is None:
+ raise NotFound("Connection not found")
+ if update_mask:
Review comment:
Also check this, https://github.com/marshmallow-code/marshmallow-sqlalchemy/issues/33#issuecomment-147008000
For updatable fields, looks like we would need it in dagruns update
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj merged pull request #9266: Add CRUD endpoint for connections
Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #9266:
URL: https://github.com/apache/airflow/pull/9266
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #9266: add crud endpoint for connections
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #9266:
URL: https://github.com/apache/airflow/pull/9266#discussion_r439877442
##########
File path: airflow/api_connexion/endpoints/connection_endpoint.py
##########
@@ -47,31 +55,53 @@ def get_connection(connection_id, session):
@provide_session
-def get_connections(session):
+def get_connections(session, limit, offset=0):
"""
Get all connection entries
"""
- offset = request.args.get(parameters.page_offset, 0)
- limit = min(int(request.args.get(parameters.page_limit, 100)), 100)
-
+ total_entries = session.query(func.count(Connection.id)).scalar()
query = session.query(Connection)
- total_entries = query.count()
- query = query.offset(offset).limit(limit)
-
- connections = query.all()
+ connections = query.offset(offset).limit(limit).all()
return connection_collection_schema.dump(ConnectionCollection(connections=connections,
total_entries=total_entries))
-def patch_connection():
+@provide_session
+def patch_connection(connection_id, session, update_mask=None):
"""
Update a connection entry
"""
- raise NotImplementedError("Not implemented yet.")
+ body = request.json
+ connection = session.query(Connection).filter_by(conn_id=connection_id).first()
+ if connection is None:
+ raise NotFound("Connection not found")
+ if update_mask:
Review comment:
Hi, check if this is ok https://github.com/apache/airflow/commit/09a7ab8be098059f1a388eeebd09610169a83117
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org