Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring from higher layers based on Protocols #3183

Open
HyeockJinKim opened this issue Nov 30, 2024 · 3 comments
Open

Refactoring from higher layers based on Protocols #3183

HyeockJinKim opened this issue Nov 30, 2024 · 3 comments
Assignees
Milestone

Comments

@HyeockJinKim
Copy link

Motivation

  1. Increased Code Complexity Due to Lack of Layer Separation
    • Currently, the code is not separated by layers, with many implementations contained within handler function
    • Understanding the upper-level business logic requires comprehending too much code, including complex lower-level implementations.
    • Difficult to find the point of origin when a bug occurs.
  2. Recurring Issues Due to Lack of Test Code
    • There is a general lack of test code, leading to frequent recurrence of similar problems.
    • An architecture where writing test code is difficult creates a vicious cycle where tests are not written.
  3. High Cost of Developing New Features
    • When developing new features, it's hard to predict what issues might occur.

Objective

  • Improve Code Readability and Maintainability Through Layer Separation
  • Refactor to a Testable Structure to Increase Code Reliability
    • Refactor into a testable structure to prevent recurring bugs and increase code reliability.
  • Support Efficient Feature Development
    • Enable easier addition of features at appropriate layers, thereby reducing the cost of developing new functionalities.

Scope

  • Layer Separation Across the Entire Codebase
    • Efficiently perform tasks by dividing epic tasks based on sub_app functionalities.
  • Sequential Refactoring from Upper Layers to Lower Layers
    • Start by defining the business logic and proceed with refactoring while defining Protocol for lower layers.
    • With separation using Protocoltest code can be written starting from the upper layers.
      • Tests for critical business logic can be written first.
  • Writing Test Code Starting from Refactored Layers
    • Sequentially write test code starting from the layers that have been refactored.

Solution

  1. Define and Separate Roles by Layer
    • Separate layers such as HandlerServiceRepository to ensure each layer has a clear role.
      • Additional layers will be defined in subsequent epics.
  2. Define Interfaces Using typing.Protocol
    • Define the interfaces needed from the upper layers first and proceed with refactoring based on them.
    • By using Protocolestablish the structure of the upper layers and gradually separate the implementations of the lower layers.
  3. Add Test Code for Refactored Functions
    • After separating tasks in the upper layersadd test code for the refactored functions.
    • This allows for early detection of errors that may occur during the refactoring process, enhancing code stability.

Roles and Responsibilities of Each Layer

To improve code maintainability and facilitate testing, the application will be structured into distinct layers, each with specific responsibilities:

1. Handler Layer

Role:

  • Acts as the entry point for incoming HTTP requests.
    Responsibilities:
  • Receive and Parse Requests:
    • Accept HTTP requests from clients.
    • Extract parameters, headers, and body content.
  • Input Validation:
    • Perform basic validation on request data (e.g., required fields, data formats).
  • Invoke Service Layer:
    • Call the appropriate service methods to process the request.
    • Transform request data into suitable formats for the service layer if necessary.
  • Handle Responses:
    • Format and send HTTP responses back to the client.
    • Set appropriate HTTP status codes and headers.
  • Error Handling:
    • Catch exceptions and translate them into HTTP responses.
    • Ensure that errors are communicated to the client in a consistent manner.
    • This action can be handled commonly by middleware.
      Notes:
  • Keep Logic Minimal:
    • The handler should contain minimal logic and should not include business logic.
  • Dependency:
    • Depends only on the Service layer interfaces.

2. Service Layer

Role:

  • Encapsulates the core business logic of the application.
    Responsibilities:
  • Implement Business Rules:
    • Apply business logic and rules to process data.
    • Ensure that domain-specific validations and computations are performed.
  • Orchestrate Operations:
    • Coordinate calls to repository, client layers and more.
    • Manage workflows that require interaction with multiple components.
  • Error Handling:
    • Manage exceptions related to business logic.
    • Raise appropriate errors to the handler layer for client communication.

Notes:

  • Framework-Agnostic:
    • Should not be dependent on web frameworks or external systems.
  • Interfaces:
    • Depends on interfaces (e.g., repository interfaces), not concrete implementations.
  • Testability:
    • Facilitates unit testing of business logic in isolation.

3. Repository Layer

Role:

  • Handles data access and persistence concerns.
    Responsibilities:
  • Data Operations:
    • Perform CRUD operations on the data store.
  • Database Interaction:
    • Interact with the database or other storage mechanisms.
    • Execute queries and handle database connections.

Notes:

  • Data source Abstraction:
    • Abstracts the data source, so the service layer doesn't need to know about database specifics.
  • Interfaces:
    • Exposes interfaces for data operations that the service layer can consume.

4. Client Layer

Role:

  • Manages communication with external services or APIs.
    Responsibilities:
  • External Communication:
    • Send requests to external systems (e.g., other microservices, third-party APIs).
    • Handle incoming responses from external services.
  • Request Handling:
    • Serialize and deserialize data when communicating with external services.
  • Error Handling and Resilience:
    • Handle errors from external services.
    • Implement retries, timeouts, circuit breakers, and fallback mechanisms.
  • Data Transformation:
    • Transform data from external formats to internal formats, and vice versa.

Notes:

  • Isolation:
    • Isolates external service differences from the rest of the application.
  • Consistency:
    • Provides a consistent interface to the service layer for interacting with external services.
  • Testability:
    • Facilitates mocking external services during testing.

I believe that starting from the upper layer by defining the interfaces for the lower layers using Protocol, and then progressively implementing the lower layers, would be an efficient approach for our refactoring process.

@HyeockJinKim HyeockJinKim self-assigned this Nov 30, 2024
@HyeockJinKim
Copy link
Author

Why this approach

1. Why start with Upper Layers?

  1. Separate Business Logic First: Focusing on separating business logic ensures that critical operations are clearly defined and makes it easier to quickly add test coverage.

  2. Incremental Separation of Layers: In situations where the required interfaces for lower layers are not yet fully understood, starting with upper layers allows for progressive refinement and better alignment.

2. Why use typing.Protocol?

  1. Faster Refactoring with Duck Typing: In scenarios where the inheritance structure is not finalized, using Protocol for duck typing enables faster and more flexible refactoring.
  2. Avoid Premature Abstractions: Protocol allow you to define required behaviors without committing to a rigid class hierarchy, reducing the risk of over-engineering during the initial stages of refactoring.
  3. Simplified Mocking: Protocols make it easier to identify and mock dependencies at upper layers, facilitating quicker testing and validation during refactoring.
  4. Freedom from Multiple Inheritance Issues: Protocols avoid the complexity and constraints often associated with multiple inheritance, making them a cleaner and more adaptable choice for interface definition.

Can you review this epic issue? @achimnol

@HyeockJinKim
Copy link
Author

Example Code (It's pseudo code)

@dataclass
class KeyPair:
    access_key: str
    secret_key: str

@dataclass
class User:
    id: str
    username: str
    role: str
    status: str
    ...

@dataclass
class AuthResult:
    user: User
    key_pair: KeyPair
    ...

class AuthServiceProtocol(Protocol):
    async def authorize(self, domain: str, username: str, password: str) -> AuthResult:
        ...

    async def get_role(self, access_key: str, group_id: str, user_id: str) -> str:
        ...

    ...

class Service:
    AuthService: AuthServiceProtocol

@attrs.define(slots=True, auto_attribs=True, init=False)
class RootContext(BaseContext):
    service: Service
    # other dependency
    ...

####

@actxmgr
async def service_ctx(root_ctx: RootContext) -> AsyncIterator[None]:
    root_ctx.service = Service.create(root_ctx)
    yield

####

async def authorize(request: web.Request, params: Any) -> web.Response:
    root_context: RootContext = request.app["_root.context"]
    service = root_context.service.AuthService
    # need to validate the parameters
    domain = params["domain"]
    username = params["username"]
    password = params["password"]
    # authorize the user
    # if user is not authorized, it will raise an exception in the service
    auth_result = await service.authorize(domain, username, password)
    return web.json_response({
        "data": {
            "access_key": auth_result.keypair["access_key"],
            "secret_key": auth_result.keypair["secret_key"],
            "role": auth_result.user["role"],
            "status": auth_result.user["status"],
        },
    })

@achimnol
Copy link
Member

achimnol commented Dec 3, 2024

LGTM! 💯

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

No branches or pull requests

2 participants