Skip to content

129: Refactoring decorators - #188

Open
JaeYeonLee0621 wants to merge 5 commits into
152-http-requests-processingfrom
129-refactor-decorators
Open

JaeYeonLee0621 wants to merge 5 commits into
152-http-requests-processingfrom
129-refactor-decorators

Conversation

@JaeYeonLee0621

@JaeYeonLee0621 JaeYeonLee0621 commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

1. Decorators refactored

Split the large gateway/views/decorators.py into a new gateway/decorators/ package with one module per concern:

  • auth.py — token_authenticated, tos_accepted, check_limits
  • body.py — parse_body, FileSizeError, ensure_usage, resolve_alias
  • availability.py — check_model_availability
  • log.py — log_request
  • errors.py — catch_router_exceptions
  • mcp.py — check_mcp_server_availability, mcp_transport_security, parse_jsonrpc_message
  • responses.py — validate_response_id, check_tool_availability and helpers
  • files.py — require_files_api_client, process_batch_file and its helpers
  • chat_completions.py — process_file_content, normalize_reasoning_fields and their helpers
  • types.py — shared type aliases
  • init.py — re-exports the decorators as a single facade

The old gateway/views/decorators.py was deleted.

2. Review opinions applied

  • check_mcp_server_availability moved into mcp.py.
  • get_relay_model_name removed (it was unused).
  • process_file_content and normalize_reasoning_fields grouped in chat_completions.py.
  • Utility functions placed next to the decorators that use them, rather than in a separate utils module.

@JaeYeonLee0621 JaeYeonLee0621 self-assigned this Sep 11, 2026
@JaeYeonLee0621
JaeYeonLee0621 marked this pull request as ready for review September 14, 2026 10:24
@meffmadd
meffmadd added this pull request to stack #190 September 15, 2026 07:20
)
from gateway.decorators.responses import check_tool_availability, validate_response_id

__all__ = [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use __all__ here? I guess we just assume we want to export everything and we still have private functions with _. This is always more work with little gain.

@JaeYeonLee0621 JaeYeonLee0621 Sep 17, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, __all__ isn't needed here, so I've removed it. External code can import the decorators using their full path.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also use it in the model refactor so you can also leave it... sorry that was probably a bit premature.

Comment thread aqueduct/gateway/decorators/auth.py Outdated
return wrapper


def check_limits(view_func: AsyncView) -> AsyncView:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think check_limits should not belong to auth. As we already have plans to expand how we handle limits, this should get its own file I think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put the check_limits definition in a new limits.py file. I'm open to suggestions on the file name or placement :)

return False


def normalize_reasoning_fields(view_func: AsyncView) -> AsyncView:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tangential but do we really only need to normalize reasoning fields for chat completions and not responses? I forgot but maybe the Responses was unified from the beginning anyways.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I fully follow this comment. Could you explain it a bit more?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were inconsistencies with the different Chat Completions API implementations where some used reasoning and others reasoning_content so we just make sure both are in the response. I was wondering if the Responses API has the same issue.

@JaeYeonLee0621 JaeYeonLee0621 Sep 21, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with this part, but from what AI said, the Responses API path doesn't go through LiteLLM's routing. It calls client.responses.create() directly on an OpenAI-compatible SDK, and the result comes back as a canonical, strongly typed Response pydantic model. So It said, It doesn't have the same issue as the Chat Completions API.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not really like the name of the file. Should we have a types.py file? Or leave it in utils.py? Basically can you check how many custom utility classes and functions we have/expect to have and split accordingly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed the file to response_type.py, since putting all these classes in a utils.py file didn't feel right and types.py seemed too generic. Let me know if you'd prefer something else and I'll change it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes ok that makes sense! Also note that this file heavily depends on 152-http-requests-processing and I wanted to check if we could rewrite the classes so that we can get rid of the middleware introduced there. So we might have to merge this first otherwise we have to merge again here etc.

Comment thread aqueduct/gateway/raw_response.py Outdated
return status_map.get(status, "invalid_request_error")


def in_wildcard(value: str | None, allowed_values: list[str]) -> bool:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably a util function right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this function to utils.py.

Comment thread aqueduct/gateway/raw_response.py Outdated
return valid


def register_response_in_cache(response_id: str | None, model: str, email: str) -> None:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the following functions are specific to the Responses API and not related to Django HTTP responses at all so these should be moved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the cache-related definitions into response_cache.py.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they should be in views/responses.py since it is so specific to the Responses endpoint. But I am not sure.

@JaeYeonLee0621 JaeYeonLee0621 Sep 21, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried making this change, but it runs into two circular imports.

Before

responses_cache.py (register/get/delete) → decorators/responses.py (validate_response_id)
responses_cache.py (register/get/delete) → views/responses.py (create_response) 
responses_cache.py (register/get/delete) → views/utils.py (ResponseRegistrationWrapper) 

After (functions moved to views/responses.py):

decorators/responses.py (validate_response_id) <-> views/responses.py (get_response_from_cache)
views/utils.py(ResponseRegistrationWrapper) <-> views/responses.py (register_response_in_cache)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants