feat: add service-info endpoint #190

JaeAeich · 2024-06-06T17:45:43Z

fixes - #157

Summary by Sourcery

This pull request introduces a new service-info endpoint to provide service information. It also refactors the application initialization process to use a new TeskApp class and adds a new configuration section for ServiceInfo in the deployment config file.

New Features:
- Introduced a new service-info endpoint to provide service information based on the ServiceInfo model.
Enhancements:
- Refactored the application initialization to use a new TeskApp class, replacing the previous FOCA-based initialization.
- Added a new configuration section for ServiceInfo in the deployment config file.

…to be looked at.

sourcery-ai · 2024-06-06T17:47:26Z

Reviewer's Guide by Sourcery

This pull request introduces a new 'service-info' endpoint to the TESK API. The changes include the addition of new models, validators, and service classes to handle the service information. The main application initialization has been refactored to use a new TeskApp class, and the configuration file has been updated to include service information details.

File-Level Changes

Files	Changes
`tesk/api/ga4gh/tes/models/tes_service_info.py` `tesk/api/ga4gh/tes/models/tes_service_info_type.py` `tesk/api/ga4gh/tes/models/tes_service_info_organization.py`	Added models to represent service information, including TesServiceInfo, TesServiceInfoType, and TesServiceInfoOrganization.
`tesk/api/ga4gh/tes/models/validators/base/base_validator.py` `tesk/api/ga4gh/tes/models/validators/rfc3339_validator.py` `tesk/api/ga4gh/tes/models/validators/rfc3986_validator.py` `tesk/api/ga4gh/tes/models/validators/rfc2386_validator.py`	Added custom validators for RFC 3339 date format, RFC 3986 URL format, and RFC 2386 email format.
`tesk/api/ga4gh/tes/service_info/base/base_service_info_request.py` `tesk/api/ga4gh/tes/service_info/service_info.py`	Added service info request handling classes, including BaseServiceInfoRequest and ServiceInfo.

sourcery-ai

Hey @JaeAeich - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

Hard-coded environment variable 'CODE_ENVIRONMENT' set to 'dev'. (link)

Here's what I looked at during the review

🟡 General issues: 5 issues found
🔴 Security: 1 blocking issue
🟢 Testing: all looks good
🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

sourcery-ai · 2024-06-06T17:47:27Z

tesk/api/ga4gh/tes/controllers.py

-			**kwargs: Arbitrary keyword arguments.
-	"""
-	pass
+def GetServiceInfo() -> dict:


suggestion (bug_risk): Consider adding error handling for service info retrieval.

The GetServiceInfo function directly returns the service info. Adding error handling or logging in case the service info retrieval fails would make the function more robust.

Suggested change

def GetServiceInfo() -> dict:

def GetServiceInfo() -> dict:

"""Get service info."""

try:

service_info = ServiceInfo()

return service_info

except Exception as e:

logging.error(f"Failed to get service info: {e}")

return {}

BaseTeskRequest handles that. 🙂

sourcery-ai · 2024-06-06T17:47:27Z

tesk/api/ga4gh/tes/models/tes_service_info.py

@@ -0,0 +1,193 @@
+"""TesServiceInfo model, used to represent the service information."""


suggestion: Consider using consistent naming conventions.

The attribute created_at uses snake_case, while updatedAt uses camelCase. For consistency, it would be better to use one naming convention throughout the model.

Suggested change

"""TesServiceInfo model, used to represent the service information."""

"""TesServiceInfo model, used to represent the service information."""

import logging

class TesServiceInfo:

def __init__(self, created_at, updated_at):

self.created_at = created_at

self.updated_at = updated_at

@uniqueg please look at this, why are there naming inconsistency in the openAPI? Is there a specific reason (can we raise a PR regarding this rn and fix it, maybe not huh 🤔 ) because this is not just some places and errs alot, I had to check the name in the yaml again and again 😢.

Yeah, there lots of inconsistencies. I hate it, too. The problem is that naming changes are generally breaking changes, so while they're easy to fix, they are very consequential changes.

But if you like, you could suggest these changes for improved consistency, but keep the old versions and add a deprecation note saying that the inconsistently named props will be removed with the next major version release. You would have my support at least.

sourcery-ai · 2024-06-06T17:47:27Z

tesk/api/ga4gh/tes/models/validators/base/base_validator.py

@@ -0,0 +1,87 @@
+"""Base validator class, all custom validator must implement it."""


suggestion: Consider adding type hints for methods.

Adding type hints for the methods in the BaseValidator class would improve code readability and help with static analysis.

Suggested change

"""Base validator class, all custom validator must implement it."""

"""Base validator class, all custom validator must implement it."""

import logging

from typing import Any

sourcery-ai · 2024-06-06T17:47:27Z

tesk/api/ga4gh/tes/service_info/base/base_service_info_request.py

@@ -0,0 +1,123 @@
+"""Base class for the TES API service info endpoint."""


suggestion (bug_risk): Consider handling thread exceptions.

The _reload_service_info method runs in a separate thread. Consider adding exception handling within the thread to ensure that any errors are logged and do not cause the thread to silently fail.

tesk/api/ga4gh/tes/service_info/service_info.py

sourcery-ai · 2024-06-06T17:47:27Z

tesk/app.py



 if __name__ == '__main__':
+	os.environ['CODE_ENVIRONMENT'] = 'dev'


🚨 issue (security): Hard-coded environment variable 'CODE_ENVIRONMENT' set to 'dev'.

Consider using a configuration file or environment variable to set the 'CODE_ENVIRONMENT' value instead of hard-coding it.

Nah, we should set the env like this, we would need this in future, if we ever come across a conditional scenario like logging or throwing an error differently based on the env.

tesk/api/ga4gh/tes/models/validators/rfc3339_validator.py

codecov · 2024-06-06T17:50:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.21%. Comparing base (c5128a9) to head (0b9aa64).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #190   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files           8        8           
  Lines         561      561           
=======================================
  Hits          551      551           
  Misses         10       10

Flag	Coverage Δ
test_unit	`98.21% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JaeAeich · 2024-06-06T17:54:42Z

Ahh, please ignore the CI fails, minor issues.

@uniqueg please review the code and some comments I left in them as well, I think I should not create a separate package base rather just have a file called base_*.py for each BaseClass, this would reduce the number of __init__.py files, my reasoning was, if BaseClass ever got complex it would have its own package level isolation where it could be dealt with but now I think they probably won't grow that big, and also why think of the problem that might not even occur.

JaeAeich · 2024-06-07T07:25:37Z

One more thing can you please check why validation errors are not being logged? Do I need to specify that in exception.py for it to be picked by FOCA?

uniqueg

I'm on the road and don't have much time, and this PR is way too big for me to have a quick check (especially on the phone), because I need to consider all implications. I'll get to it when I have time, but we are all attending a conference next week, so either you wait and do sth else in the meantime, or you break down the PR into smaller pieces that can be more easily reviewed.

For example you could start with just the base classes. Then the models. And so on.

uniqueg · 2024-06-07T13:13:16Z

deployment/config.yaml

+  version: 1.0.0
+  storage:
+    - file:///path/to/local/funnel-storage
+    - s3://ohsu-compbio-funnel/storage


Maybe also put a dummy value here

uniqueg · 2024-06-07T13:15:28Z

deployment/config.yaml

@@ -48,6 +48,31 @@ api:
          swagger_ui: True
          serve_spec: True

+# ServiceInfo configuration based on ServiceInfo model
+serviceInfo:


Consider adding this in the custom section. Then you could also provide a module with Pydantic models and have FOCA validate these config params. I think this is done in proTES and maybe some other services that use a recent FOCA version.

uniqueg · 2024-06-07T13:21:14Z

tesk/api/ga4gh/tes/models/tes_service_info.py

@@ -0,0 +1,193 @@
+"""TesServiceInfo model, used to represent the service information."""


Yeah, there lots of inconsistencies. I hate it, too. The problem is that naming changes are generally breaking changes, so while they're easy to fix, they are very consequential changes.

But if you like, you could suggest these changes for improved consistency, but keep the old versions and add a deprecation note saying that the inconsistently named props will be removed with the next major version release. You would have my support at least.

uniqueg · 2024-06-21T12:24:01Z

One more thing can you please check why validation errors are not being logged? Do I need to specify that in exception.py for it to be picked by FOCA?

Yes. And map it to a HTTP response code, e.g., 400.

Also note that currently only exceptions raised in app context are logged through FOCA. For errors occuring outside of the request-response cycle, I think FOCA will not do anything and probably just a traceback is dumped to the logs. I will see if I can make this more consistent for the next FOCA release (which requires considerable changes on error handling).

uniqueg · 2024-06-21T12:25:28Z

Ahh, please ignore the CI fails, minor issues.

@uniqueg please review the code and some comments I left in them as well, I think I should not create a separate package base rather just have a file called base_*.py for each BaseClass, this would reduce the number of __init__.py files, my reasoning was, if BaseClass ever got complex it would have its own package level isolation where it could be dealt with but now I think they probably won't grow that big, and also why think of the problem that might not even occur.

If the base class(s) is(are) sufficiently abstract (i.e., no ties to anything GA4GH related), we could also add it(them) to FOCA.

I will review the design in the other PR, I guess (I'm a bit confused about the PRs).

JaeAeich · 2024-06-21T12:26:46Z

Don't Review this PR @uniqueg!!! Move on to #192 #191

This is the prev implementation

JaeAeich · 2024-06-30T07:59:24Z

please check #198

JaeAeich added 4 commits June 4, 2024 00:15

add models and validators

7302f89

endpoint seem to be working, validation not so much, threading still …

698fd62

…to be looked at.

validators working ٩(^ᗜ^ )و

df46853

reload thingy ʘ‿ʘ

eb9da5a

sourcery-ai bot reviewed Jun 6, 2024

View reviewed changes

JaeAeich requested a review from uniqueg June 6, 2024 17:54

ci ʘ‿ʘ

27506c5

JaeAeich force-pushed the feat/service-info branch from 5b0e58d to 27506c5 Compare June 7, 2024 11:30

nitpick ( ˘▽˘)っ♨

0b9aa64

uniqueg reviewed Jun 7, 2024

View reviewed changes

JaeAeich closed this Jun 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add service-info endpoint #190

feat: add service-info endpoint #190

JaeAeich commented Jun 6, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jun 6, 2024

sourcery-ai bot left a comment

sourcery-ai bot Jun 6, 2024

JaeAeich Jun 7, 2024

sourcery-ai bot Jun 6, 2024

JaeAeich Jun 7, 2024

uniqueg Jun 7, 2024

sourcery-ai bot Jun 6, 2024

sourcery-ai bot Jun 6, 2024

sourcery-ai bot Jun 6, 2024

JaeAeich Jun 7, 2024

codecov bot commented Jun 6, 2024 •

edited

Loading

JaeAeich commented Jun 6, 2024

JaeAeich commented Jun 7, 2024

uniqueg left a comment

uniqueg Jun 7, 2024

uniqueg Jun 7, 2024

uniqueg Jun 7, 2024

uniqueg commented Jun 21, 2024

uniqueg commented Jun 21, 2024 •

edited

Loading

JaeAeich commented Jun 21, 2024

JaeAeich commented Jun 30, 2024

-def GetServiceInfo() -> dict:
+def GetServiceInfo() -> dict:
+    """Get service info."""
+    try:
+        service_info = ServiceInfo()
+        return service_info
+    except Exception as e:
+        logging.error(f"Failed to get service info: {e}")
+        return {}

		@@ -0,0 +1,193 @@
		"""TesServiceInfo model, used to represent the service information."""

-"""TesServiceInfo model, used to represent the service information."""
+"""TesServiceInfo model, used to represent the service information."""
+import logging
+class TesServiceInfo:
+    def __init__(self, created_at, updated_at):
+        self.created_at = created_at
+        self.updated_at = updated_at

		@@ -0,0 +1,87 @@
		"""Base validator class, all custom validator must implement it."""

		@@ -0,0 +1,123 @@
		"""Base class for the TES API service info endpoint."""



		if __name__ == '__main__':
		os.environ['CODE_ENVIRONMENT'] = 'dev'

feat: add service-info endpoint #190

feat: add service-info endpoint #190

Conversation

JaeAeich commented Jun 6, 2024 • edited by sourcery-ai bot Loading

Summary by Sourcery

sourcery-ai bot commented Jun 6, 2024

Reviewer's Guide by Sourcery

File-Level Changes

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Jun 6, 2024

Choose a reason for hiding this comment

JaeAeich Jun 7, 2024

Choose a reason for hiding this comment

sourcery-ai bot Jun 6, 2024

Choose a reason for hiding this comment

JaeAeich Jun 7, 2024

Choose a reason for hiding this comment

uniqueg Jun 7, 2024

Choose a reason for hiding this comment

sourcery-ai bot Jun 6, 2024

Choose a reason for hiding this comment

sourcery-ai bot Jun 6, 2024

Choose a reason for hiding this comment

sourcery-ai bot Jun 6, 2024

Choose a reason for hiding this comment

JaeAeich Jun 7, 2024

Choose a reason for hiding this comment

codecov bot commented Jun 6, 2024 • edited Loading

Codecov Report

JaeAeich commented Jun 6, 2024

JaeAeich commented Jun 7, 2024

uniqueg left a comment

Choose a reason for hiding this comment

uniqueg Jun 7, 2024

Choose a reason for hiding this comment

uniqueg Jun 7, 2024

Choose a reason for hiding this comment

uniqueg Jun 7, 2024

Choose a reason for hiding this comment

uniqueg commented Jun 21, 2024

uniqueg commented Jun 21, 2024 • edited Loading

JaeAeich commented Jun 21, 2024

JaeAeich commented Jun 30, 2024

JaeAeich commented Jun 6, 2024 •

edited by sourcery-ai bot

Loading

codecov bot commented Jun 6, 2024 •

edited

Loading

uniqueg commented Jun 21, 2024 •

edited

Loading