Skip to content

fix!: merge payload and data fields of Request#542

Merged
vdusek merged 7 commits intomasterfrom
merge-payload-and-data
Oct 24, 2024
Merged

fix!: merge payload and data fields of Request#542
vdusek merged 7 commits intomasterfrom
merge-payload-and-data

Conversation

@vdusek
Copy link
Copy Markdown
Collaborator

@vdusek vdusek commented Sep 24, 2024 •
edited
Loading

Description

  • We had data and payload fields on the Request model.
    • payload was not being provided to the HTTP clients, only the data field.
  • In this PR, I'm merging them together, keeping only the data field (use the same naming as HTTPX & CurlImpersonate).
  • In this PR, I'm merging them together, keeping only the payload field (use the same naming as in JS Crawlee).
  • I also defined type aliases for HTTP data and HTTP query parameters and used them across the project.
  • Some struggle with Pydantic & serialization, but should be OK now...

Issues

Testing

  • The current set of unit tests should cover the changes.

Checklist

  • CI passed

@vdusek vdusek added bug Something isn't working. t-tooling Issues with this label are in the ownership of the tooling team. adhoc Ad-hoc unplanned task added during the sprint. labels Sep 24, 2024
@vdusek vdusek added this to the 99th sprint - Tooling team milestone Sep 24, 2024
@vdusek vdusek requested a review from janbuchar September 24, 2024 10:50
@vdusek vdusek self-assigned this Sep 24, 2024
@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Sep 24, 2024
@B4nan
Copy link
Copy Markdown
Member

B4nan commented Sep 24, 2024

JS version have only payload, we should aim for compact with that in the first place.

@B4nan
Copy link
Copy Markdown
Member

B4nan commented Sep 24, 2024

Also, since this is breaking, maybe we should wait, and merge it once we have more breaking changes than a simple one like this (or some feature worth the minor bump).

@vdusek vdusek force-pushed the merge-payload-and-data branch from 9d83dcb to 49243db Compare September 24, 2024 11:42
@honzajavorek
Copy link
Copy Markdown
Contributor

honzajavorek commented Sep 24, 2024

I learned about data here, but I don't see it in changed files: https://crawlee.dev/python/docs/examples/fill-and-submit-web-form#preparing-a-post-request

@honzajavorek
Copy link
Copy Markdown
Contributor

honzajavorek commented Sep 24, 2024

It also says "Alternatively, you can send form data as URL parameters using the query_params argument." btw, not sure if that is something related or not.

@vdusek
Copy link
Copy Markdown
Collaborator Author

vdusek commented Sep 24, 2024

Also, since this is breaking, maybe we should wait, and merge it once we have more breaking changes than a simple one like this (or some feature worth the minor bump).

We will have the first version of fingerprints for Playwright soon 🤞.

I learned about data here, but I don't see it in changed files: https://crawlee.dev/python/docs/examples/fill-and-submit-web-form#preparing-a-post-request

Thanks. Of course, I forgot to update the docs.

janbuchar
janbuchar requested changes Sep 24, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@janbuchar janbuchar left a comment

Choose a reason for hiding this comment

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

Nice! Just a couple of questions...

@vdusek vdusek force-pushed the merge-payload-and-data branch from c04ecb2 to 414ac66 Compare September 24, 2024 13:12
@vdusek vdusek requested a review from janbuchar September 24, 2024 13:56
@vdusek vdusek force-pushed the merge-payload-and-data branch 3 times, most recently from 649eaa5 to 4c3d891 Compare September 24, 2024 14:09
@vdusek vdusek mentioned this pull request Sep 25, 2024
Merged
1 task
@vdusek
Copy link
Copy Markdown
Collaborator Author

vdusek commented Sep 25, 2024

Also, since this is breaking, maybe we should wait, and merge it once we have more breaking changes than a simple one like this (or some feature worth the minor bump).

I've separated the Curl impersonation fix and other minor updates into a separate PR (#543), allowing us to release a patch version for 0.3.

The payload & data merge and HttpHeaders thing will be resolved later...

vdusek added a commit that referenced this pull request Sep 25, 2024
@vdusek
fix: adopt new version of curl-cffi (#543)
f6fcf48 
### Description

- Version 1.7.2 of `curl-cffi` introduces breaking changes.
- This update includes adopting the new version, adding type aliases for
`Request` fields, and incorporating other minor changes from PR #542.

### Issues

- N/A (ad-hoc fix)

### Testing

- The current set of unit tests should cover the changes.

### Checklist

- [x] CI passed
@vdusek vdusek force-pushed the merge-payload-and-data branch from 4c3d891 to f0ea79c Compare September 25, 2024 07:38
@vdusek vdusek marked this pull request as draft September 25, 2024 07:39
This was referenced Sep 25, 2024
Merged
Closed
@vdusek vdusek removed adhoc Ad-hoc unplanned task added during the sprint. bug Something isn't working. t-tooling Issues with this label are in the ownership of the tooling team. labels Oct 2, 2024
@vdusek vdusek removed this from the 99th sprint - Tooling team milestone Oct 2, 2024
@vdusek vdusek force-pushed the merge-payload-and-data branch from 62b332e to c371b29 Compare October 2, 2024 15:01
@vdusek vdusek force-pushed the merge-payload-and-data branch from c371b29 to d1b9abb Compare October 22, 2024 08:45
@vdusek
Copy link
Copy Markdown
Collaborator Author

vdusek commented Oct 22, 2024

A new model for HttpHeaders was also moved to a separate PR #544 and merged.

@vdusek vdusek force-pushed the merge-payload-and-data branch from d1b9abb to 2752643 Compare October 22, 2024 14:14
@vdusek vdusek marked this pull request as draft October 22, 2024 14:14
@vdusek vdusek force-pushed the merge-payload-and-data branch from 2752643 to 260acd1 Compare October 23, 2024 14:38
@vdusek vdusek marked this pull request as ready for review October 23, 2024 14:39
@vdusek vdusek mentioned this pull request Oct 23, 2024
Merged
1 task
janbuchar
janbuchar requested changes Oct 23, 2024
View reviewed changes
@vdusek vdusek requested a review from janbuchar October 23, 2024 15:28
janbuchar
janbuchar requested changes Oct 23, 2024
View reviewed changes
@janbuchar
Copy link
Copy Markdown
Collaborator

janbuchar commented Oct 23, 2024

Nice work @vdusek, and sorry for taking multiple rounds of review to pinpoint all issues.

@vdusek vdusek mentioned this pull request Oct 24, 2024
Closed
@vdusek vdusek requested review from janbuchar and removed request for janbuchar October 24, 2024 08:42
janbuchar
janbuchar reviewed Oct 24, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@janbuchar janbuchar left a comment

Choose a reason for hiding this comment

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

Looking good, just a nit.

@vdusek vdusek requested a review from janbuchar October 24, 2024 10:54
@vdusek vdusek merged commit d06fcef into master Oct 24, 2024
@vdusek vdusek deleted the merge-payload-and-data branch October 24, 2024 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@janbuchar janbuchar janbuchar approved these changes

Assignees

@vdusek vdusek

Labels

t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Milestone

101st sprint - Tooling team

Development

Successfully merging this pull request may close these issues.

Unable to execute POST request with JSON payload

4 participants

@vdusek @B4nan @honzajavorek @janbuchar