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

[autoendpoint] Flatten module structure a bit #157

Closed
AzureMarker opened this issue Jun 16, 2020 · 3 comments · Fixed by #182
Closed

[autoendpoint] Flatten module structure a bit #157

AzureMarker opened this issue Jun 16, 2020 · 3 comments · Fixed by #182
Assignees
Labels
1 Estimate - Trivial

Comments

@AzureMarker
Copy link
Contributor

This is a real small issue, but I'm making an issue here to track it. Additionally, if we use the autoendpoint crate as a template in the future, we want it to be well architected.

The server module is a little too big. It should only handle setting up the Actix server. The extractors, routes, business logic, etc should be contained in "peer" modules.

This change should be done once there are no "major" PRs open for autoendpoint (perhaps after #154).

Current layout:

├── server
│   ├── extractors
│   │   ├── mod.rs
│   │   ├── notification.rs
│   │   ├── notification_headers.rs
│   │   ├── subscription.rs
│   │   └── token_info.rs
│   ├── headers
│   │   ├── crypto_key.rs
│   │   ├── mod.rs
│   │   ├── util.rs
│   │   └── vapid.rs
│   ├── mod.rs
│   └── routes
│       ├── health.rs
│       ├── mod.rs
│       └── webpush.rs

Proposed layout:

├── extractors
│   ├── mod.rs
│   ├── notification.rs
│   ├── notification_headers.rs
│   ├── subscription.rs
│   └── token_info.rs
├── headers
│   ├── crypto_key.rs
│   ├── mod.rs
│   ├── util.rs
│   └── vapid.rs
├── routes
│   ├── health.rs
│   ├── mod.rs
│   └── webpush.rs
└── server.rs
@AzureMarker AzureMarker added the 1 Estimate - Trivial label Jun 16, 2020
@AzureMarker AzureMarker added this to the Autoendpoint Rust Server milestone Jun 16, 2020
@AzureMarker AzureMarker self-assigned this Jun 16, 2020
@pjenvey
Copy link
Member

pjenvey commented Jun 16, 2020

👍 this is closer to syncstorage-rs's layout, with the major difference being it puts most of these bits under a web/ sub dir.

At the risk of bike-shedding: syncstorage-rs calls the routes equivalent here "handlers" -- might want consider this naming to be consistent across the code bases.

I guess technically the Route in actix-web is more of the middleman between the Service layer and the actual handlers?

@AzureMarker
Copy link
Contributor Author

Bike-shedding (when productive) is part of this issue's purpose! I'm a little hesitant about using handlers because it is more general than routes. Using one vs another probably depends upon if we write all of the "handler" code directly in the route functions, or if the route functions are just bridging the gap between the HTTP interface and our business logic layer (like the middlemen you mentioned). How that code will look isn't clear yet, so for now I think both names are good.

@jrconlin
Copy link
Member

jrconlin commented Jun 17, 2020

<soapbox>
FWIW, I tend not to favor "clever" code. Things like decorators or rusts auto-instantiated variables are examples. These potentially introduce subtle side effects and generally obscure where the data originates unless someone is already very familiar with either the framework or the codebase. (For auto-instantiated variables, are those created automagically, or being passed in?)

The same can hold with functions. routes should be about handling structs based on user requests, extractors should be about generating structs derived from other structs (e.g. generating the VAPID info from the Request structure). routes would (ideally, explicitly) derive objects to be processed, and then call whatever proper methods are required to deal with things.

Where things could get "fuzzy" is around things that are outside of those simple flows. For instance, how would data storage factor in? Should it be an explicit component that could be "swapped" later or be implicit in the route or extraction processes work? I tend to favor simplicity and explicity over clever mostly because that's what I would want when I look at the code again in a few months/years working on very different things and have forgotten all the cleverness.

</soapbox>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 Estimate - Trivial
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants