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

Consider making web::Path and web::Query fields public again #2016

Closed
luisholanda opened this issue Feb 19, 2021 · 6 comments · Fixed by #2017
Closed

Consider making web::Path and web::Query fields public again #2016

luisholanda opened this issue Feb 19, 2021 · 6 comments · Fixed by #2017
Labels
A-web project: actix-web C-chore Category: chore good-first-issue easy to pick up for newcomers
Milestone

Comments

@luisholanda
Copy link

luisholanda commented Feb 19, 2021

Hello! First of all thanks for the awesome work being done in the libraries!

While updating our company's codebase to tokio v1 (and thus actix-web v4) we noticed that #1894 made the field of web::Path and web::Query private. This change alone broke almost all our services' routes.

Given that the web::Json and web::Form still have their field public, it would be possible to rollback the change made in the former types? Having the fields private is a big burden for us, having to call .into_inner() everywhere is pretty annoying.

I can send a PR if this change is desired by the maintainers.

Your Environment

  • Rust Version (I.e, output of rustc -V): nightly-2021-01-25
  • Actix Web Version: 4.0.0-beta.3
@luisholanda luisholanda changed the title Consider making web::Path and web::Query field public again Consider making web::Path and web::Query fields public again Feb 19, 2021
@robjtede
Copy link
Member

robjtede commented Feb 19, 2021

It has been considered a mistake to make the Path inner field public in v3 for reasons that do not affect Json or Form or Query.

Given Path is primarily used with tuples, a public inner field was making it quite confusing for developers that there was a difference between path.0 -> (String, String) and path.1 -> String. The possible solutions to this discontinuity are to either remove the public field or remove the Deref impl. The former was chosen as to affect fewer use cases.

It's like the Query public inner field was removed by mistake, or else I do not recall the rationale for it.

@luisholanda
Copy link
Author

The rationale makes sense. Unfortunately, in our codebase, we destruct web::* types as soon as we receive them, so the change hit us really hard.

If most users make use of the Deref impl, I also think is better to keep it and we will rollback to using .into_inner().

It's like the Query public inner field was removed by mistake, or else I do not recall the rationale for it.

If so, it is possible to make at least it's field public?

@robjtede robjtede added this to the actix-web v4 milestone Feb 19, 2021
@robjtede robjtede added C-chore Category: chore A-web project: actix-web good-first-issue easy to pick up for newcomers labels Feb 19, 2021
adamchalmers added a commit to adamchalmers/actix-web that referenced this issue Feb 20, 2021
adamchalmers added a commit to adamchalmers/actix-web that referenced this issue Feb 20, 2021
adamchalmers added a commit to adamchalmers/actix-web that referenced this issue Feb 20, 2021
robjtede added a commit that referenced this issue Feb 20, 2021
Co-authored-by: Rob Ede <robjtede@icloud.com>
@dxps
Copy link

dxps commented Aug 8, 2021

Hi everyone,

I am using actix-web = "4.0.0-beta.8" and if this aspect has been solved,
then can you tell me please why such approach does not work anymore (as it is working in v3) ❔
Thanks.

In v3:

pub async fn some_handler(
    web::Path(tutor_id, course_id): web::Path<(i32, i32)>,
) -> ... {

In v4: we have to just mention some params and destructure it:

pub async fn some_handler(
    params: web::Path<(i32, i32)>,
) -> ... {
    let (tutor_id, course_id) = params.into_inner();

Update:
Ok, it seems that what @robjtede mentioned is still the decision for this change.
I'd say at least to have such (quite impactful, right?) change explicit in the Migration to v4, at least.

@robjtede
Copy link
Member

robjtede commented Aug 8, 2021

I'd say at least to have such change explicit in the Migration to v4, at least.

Yep we're going to fill in the migration guide before stable release.

@ModProg
Copy link
Contributor

ModProg commented Mar 20, 2022

To be able to update one of my projects with little hassle, I created this https://github.com/ModProg/actix-web-public-path .

Still think it is very nice to be able to declare your request like functions and get your path arguments directly in the declaration without need for an additional into_inner but I understand the reasoning behind the decision.

@robjtede
Copy link
Member

Should have mentioned it here but there's a variant with public field in actix-web-lab, too.

https://docs.rs/actix-web-lab/0.15.0/actix_web_lab/extract/struct.Path.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web project: actix-web C-chore Category: chore good-first-issue easy to pick up for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants