bug/Fixes #32
bug/Fixes #32
Conversation
Reviewer's Guide by SourceryThis pull request makes significant changes to an API service, including updates to the OpenAPI specification, error handling, parameter validation, and various utility functions. The changes aim to improve the robustness and functionality of the API, with a focus on better request handling, validation, and error reporting. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @dan5e3s6ares - I've reviewed your changes - here's some feedback:
Overall Comments:
- This PR makes significant improvements to error handling, parameter validation, and API robustness. However, consider breaking future large changes into smaller, more focused PRs to facilitate easier review.
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| ) | ||
|
|
||
| payload = {} | ||
|
|
||
| try: |
There was a problem hiding this comment.
suggestion: Improved error handling for required payloads has been added.
This is a good improvement in input validation and error reporting. Consider adding more specific error messages to help API consumers understand exactly what went wrong.
try:
payload = await request.json()
except json.decoder.JSONDecodeError:
raise HTTPException(status_code=400, detail="Invalid JSON payload")
if from_function.get("payload", {}).get("required", False) and not payload:
raise HTTPException(status_code=400, detail="Payload is required")
try:
validate(instance=payload, schema=from_function.get("payload", {}).get("schema", {}))
except ValidationError as e:
raise HTTPException(status_code=400, detail=f"Validation error: {str(e)}")
| def read_file(cls): | ||
| data = None | ||
| with open("files/openapi.json", "r", encoding="utf-8") as file: | ||
| return json.load(file) | ||
| data = json.load(file) | ||
| return data |
There was a problem hiding this comment.
suggestion: Added null checking when reading the JSON file.
This is a good defensive programming practice. Consider adding more specific error handling or logging if the file is empty or cannot be parsed.
@classmethod
def read_file(cls):
try:
with open("files/openapi.json", "r", encoding="utf-8") as file:
data = json.load(file)
if not data:
raise ValueError("Empty JSON file")
return data
except (IOError, json.JSONDecodeError, ValueError) as e:
logging.error(f"Error reading JSON file: {e}")
return None
| $ref: "#/components/schemas/Podcast" | ||
| description: OK | ||
| $ref: '#/components/schemas/ResponseError' | ||
| security: |
There was a problem hiding this comment.
🚨 suggestion (security): Provide more details on OAuth2 implementation and required scopes
The security scheme uses OAuth2, which is good, but more details on the implementation and required scopes for each endpoint would be helpful for API consumers.
| security: | |
| security: | |
| - OAuth2Security: | |
| - quote-patrimonial-lead | |
| x-oauth2-scopes: | |
| quote-patrimonial-lead: | |
| description: "Access to create and manage patrimonial lead quotes" | |
| read:quotes: | |
| description: "Read-only access to quotes" | |
| write:quotes: | |
| description: "Full access to create and modify quotes" |
| portableType: | ||
| description: Modalides de Equipamentos Portáteis | ||
| type: string | ||
|
|
There was a problem hiding this comment.
suggestion: Consider refactoring repeated structures across different quote request types
The current structure has significant repetition across QuoteRequestPatrimonialHome, QuoteRequestPatrimonialCondominium, QuoteRequestPatrimonialBusiness, and QuoteRequestPatrimonialDiverseRisks. Consider creating a base QuoteRequest schema and extending it for specific types to improve maintainability and reduce duplication.
BaseQuoteRequest:
type: object
properties:
meta:
$ref: '#/components/schemas/Meta'
QuoteRequestPatrimonialDiverseRisks:
allOf:
- $ref: '#/components/schemas/BaseQuoteRequest'
- type: object
| maxLength: 10 | ||
| format: date | ||
| pattern: '^(\d{4})-(1[0-2]|0?[1-9])-(3[01]|[12][0-9]|0?[1-9])$' | ||
| example: '2022-10-02' | ||
|
|
||
| invoiceNumber: | ||
| description: Número da Nota Fiscal | ||
| type: number | ||
|
|
||
| isRented: | ||
| description: Equipamento é alugado? | ||
| type: boolean | ||
|
|
||
| isBorrowed: | ||
| description: Equipamento é locado/cedido a terceiros? | ||
| type: boolean | ||
|
|
||
| isFinanced: | ||
| description: Equipamento é arrendado ou financiado? | ||
| type: boolean | ||
|
|
||
| ownOperation: | ||
| description: A utilização/operação do equipamento é feita exclusivamente pelo proprietário e/ou seus funcionários? | ||
| type: boolean | ||
|
|
||
| operationLocation: | ||
| description: Equipamento opera em local fechado ou ao ar-livre? | ||
| type: string | ||
| enum: [ | ||
| LOCAL_FECHADO, | ||
| AR-LIVRE] | ||
|
|
||
| buildingSiteOperation: | ||
| description: Equipamento está operando em canteiro de obra? | ||
| type: boolean | ||
|
|
||
| closeToWaterOperation: | ||
| description: Opera próximo a rio, mar ou lago? | ||
| type: boolean | ||
|
|
||
| onWaterOperation: | ||
| description: Opera sobre a água? | ||
| type: boolean | ||
|
|
||
| hasTracker: | ||
| description: Os equipamentos possuem GPS ou rastreador instalado? | ||
| type: boolean | ||
|
|
||
| undergroundWork: | ||
| description: Equipamento é utilizado em Obras Subterrânes e/ou escavação de túneis? | ||
| type: boolean | ||
|
|
||
| ### | ||
| fixedOnVehicles: | ||
| description: Equipamentos são acoplados, fixados ou instalados a veículos? | ||
| type: boolean | ||
| PortableEquipment: | ||
| type: object | ||
| description: Objeto que agrupa os dados de Equipamentos Portáteis. | ||
| properties: | ||
| portableType: | ||
| description: Modalides de Equipamentos Portáteis | ||
| type: string | ||
|
|
There was a problem hiding this comment.
suggestion: Consider extracting common header definitions
The x-fapi-interaction-id header is defined multiple times with identical content. Could this be extracted into a reusable component to reduce duplication and improve maintainability? Is there a specific reason for keeping these definitions separate?
components:
schemas:
StandardErrorResponse:
type: object
properties:
content:
application/json; charset=utf-8:
schema:
$ref: '#/components/schemas/ResponseError'
responses:
Forbidden:
$ref: '#/components/schemas/StandardErrorResponse'
description: O token tem escopo incorreto ou uma política de segurança foi violada
InternalServerError:
$ref: '#/components/schemas/StandardErrorResponse'
description: Ocorreu um erro no gateway da API ou no microsserviço
| @@ -50,9 +50,13 @@ def build(cls, schema: dict): | |||
| key = next(iter(schema['content'])) | |||
| study_schema = schema['content'][key]["schema"] | |||
| base_schema = cls.turn_schema({}, study_schema) | |||
| required = False | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring the logic for extracting the 'required' field into its own method.
The recent changes introduce a required variable and additional logic to handle it, which adds complexity to the build method. To improve readability and maintainability, consider refactoring by separating the logic for extracting the "required" field into its own method. This will make the build method cleaner and easier to understand. Here's a suggestion:
@classmethod
def build(cls, schema: dict):
if "content" in schema:
key = next(iter(schema['content']))
study_schema = schema['content'][key]["schema"]
base_schema = cls.turn_schema({}, study_schema)
required = cls.extract_required(schema)
return {
"content_type": key,
"schema": base_schema,
"required": required,
}
if "$ref" in schema:
base = cls.turn_schema({}, cls.find_schema(schema))
return cls.build(base)
return {"content_type": "*/*", "description": schema["description"]}
@staticmethod
def extract_required(schema: dict) -> bool:
return schema.get("required", False)This refactoring keeps the functionality intact while making the code more modular and easier to maintain.
| @@ -15,16 +15,17 @@ class CheckBodyVariableValue: | |||
|
|
|||
| @classmethod | |||
| async def response(cls, request: Request, url_data: dict): | |||
| payload = await request.json() | |||
There was a problem hiding this comment.
issue (complexity): Consider maintaining the original logic of checking 'payload_id' for clarity.
The new code introduces some complexity compared to the original. The logic now checks if "123" is in the alias_id from the URL path, which can be less intuitive and potentially error-prone. The original code's approach of checking payload_id from the JSON body is more straightforward. Additionally, there's an inconsistency with error handling: the schema mentions 422, but the code returns 405. For clarity and maintainability, consider sticking to the original logic of checking payload_id and ensure consistent use of status codes. Here's a simplified version:
class CheckBodyVariableValue:
@classmethod
async def response(cls, request: Request, url_data: dict):
payload = await request.json()
if payload.get('payload_id') == "123":
schema_key = "200"
status_code = 200
else:
schema_key = "405"
status_code = 405
faker = JSF(url_data['responses'][schema_key]['schema'])
fake_payload_response = faker.generate(
n=1, use_defaults=True, use_examples=True
)
return JSONResponse(fake_payload_response, status_code=status_code)This maintains the original logic, ensuring clarity and consistency.
| schema=from_function["payload"]["schema"], | ||
| ) | ||
|
|
||
| if required and payload == {}: |
There was a problem hiding this comment.
suggestion (code-quality): Replaces an empty collection equality with a boolean operation (simplify-empty-collection-comparison)
| if required and payload == {}: | |
| if required and not payload: |
| missing_fields = [] | ||
|
|
||
| for index, item in enumerate(rules_dict["required"]): | ||
| for _, item in enumerate(rules_dict["required"]): | ||
| if item.lower() not in params_dict_keys: | ||
| missing_fields.append( | ||
| {"msg": "Missing Field", "loc": [index, item]} | ||
| {"msg": "Missing Field", "loc": [position, item]} | ||
| ) | ||
|
|
||
| if len(missing_fields) > 0: |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Convert for loop into list comprehension (
list-comprehension) - Remove unnecessary calls to
enumeratewhen the index is not used (remove-unused-enumerate) - Simplify sequence length comparison (
simplify-len-comparison) - Use named expression to simplify assignment and conditional (
use-named-expression)
| missing_fields = [] | |
| for index, item in enumerate(rules_dict["required"]): | |
| for _, item in enumerate(rules_dict["required"]): | |
| if item.lower() not in params_dict_keys: | |
| missing_fields.append( | |
| {"msg": "Missing Field", "loc": [index, item]} | |
| {"msg": "Missing Field", "loc": [position, item]} | |
| ) | |
| if len(missing_fields) > 0: | |
| if missing_fields := [ | |
| {"msg": "Missing Field", "loc": [position, item]} | |
| for item in rules_dict["required"] | |
| if item.lower() not in params_dict_keys | |
| ]: |
| required = False | ||
| if "required" in schema: | ||
| required = schema['required'] |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Move setting of default value for variable into
elsebranch (introduce-default-else) - Replace if statement with if expression (
assign-if-exp)
| required = False | |
| if "required" in schema: | |
| required = schema['required'] | |
| required = schema['required'] if "required" in schema else False |
Fix a lot of bugs
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by Sourcery
Revamp the API to support the 'API Quote Patrimonial - Open Insurance Brasil' with new endpoints for various insurance products, improve schema validation by handling required fields, enhance error reporting for missing headers, and update the mock API configuration.
New Features:
Bug Fixes:
Enhancements:
Build: