Skip to content

Update ARCHITECTURE.md with latest revisions and high-level proposed HTTP approach #46

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

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

felixarntz
Copy link
Member

This PR revises a few things in ARCHITECTURE.md, partially in accordance with revisions already implemented, partially as a proposed approach based on recent conversations about the HTTP infrastructure layer.

Here's a breakdown:

  • Update the interface for operations based on recent code revisions. See b409875
  • Update the file API layer based on how it was eventually implemented. See 85eea9c
  • Fix minor incorrection with function call and function response DTOs. See 8a200f7
  • Revise proposed HTTP infrastructure, and related authentication layer. See dd245b1
    • I created this after reviewing the in progress work in trunk...add-http-client-and-auth from @JasonTheAdams, with minor modifications. Let's discuss these here.
    • This is supposed to be only a high-level summary in line with the rest of the existing class diagrams, i.e. not as detailed as what that branch is adding.

Note: There are 3 very minor fixes in actual code files that I noticed were needed while reviewing the architecture diagram.

@felixarntz felixarntz added the [Type] Developer Documentation Documentation for developers label Aug 11, 2025
@felixarntz
Copy link
Member Author

@JasonTheAdams Let's discuss the minor differences between your proposed HTTP architecture outline in trunk...add-http-client-and-auth and what I have here:

  1. I think all of it should live within the Providers namespace, in an Http sub-namespace of it. That's because they're purely relevant for the extender API, and putting all such code into Providers was something we initially went for.
  2. I included a getData method in Request and Response, which makes it easy to work with certain common payloads. How that data is handled under the hood is an implementation detail.
    • For example, in a GET request, get_data() would return the key value pairs for what's going to be eventually sent as query parameters.
    • For example, in a JSON response, get_data() would return the associative array of JSON-decoded data.
    • When irrelevant, e.g. in a response for a binary file, get_data() would simply return null.
  3. getBody may return null, as it may not always be present. The idea is that depending on the type of request or response, you either expect a body (raw string) or data (associative data array).
  4. getHeaders always returns array<string, string[]>, instead of having the values be string|string[], which is error-prone as it would require any caller to check whether the data is an array or a single string. Choosing one and always going with it is more straightforward to use.
  5. I left out the more specific getReasonPhrase and isSuccessful methods in Response. IMO they're too opinionated to include in the main DTO, at least for now.
  6. I left out the with* methods in Request. I'm not sure we need them, and if we actually do, this would be a new pattern we don't have anywhere else. Maybe this is how we would want to do it, maybe have a RequestBuilder instead. Anyway, I think this would be a premature discussion at this point, since again I'm not sure we even need it.

Let me know your thoughts on these changes.

FWIW I also revised Authentication to be more clearly tied to the Http infrastructure, also by renaming it to RequestAuthentication. This is not a change from what your PR is doing, since it doesn't touch those classes, but still wanted to highlight it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant