Skip to content
This repository has been archived by the owner on Apr 16, 2022. It is now read-only.

Make HTTP POST requests repeatable #856

Merged
merged 1 commit into from
Mar 13, 2020
Merged

Make HTTP POST requests repeatable #856

merged 1 commit into from
Mar 13, 2020

Conversation

ggalmazor
Copy link
Contributor

@ggalmazor ggalmazor commented Mar 11, 2020

Make HTTP post requests repeatable by wrapping entities into a repeatable entity

In order to test this PR with Ona servers, https://stage-odk.ona.io must be used because it looks like the production servers haven't been patched yet.

Closes #852

What has been done to verify that this works as intended?

@DavisRayM has tested that this PR fixes the issues he's experiencing while pushing forms to Ona.

Why is this the best possible solution? Were any other approaches considered?

The changes in this PR come straight up from Apache HttpClient's documentation & examples, so no alternatives have been considered.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

The change is super narrow, so I don't expect any nasty side-effects, although manual tests should be extended to Aggregate and Central servers just to be sure nothing broke.

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

Nope.

@ggalmazor ggalmazor marked this pull request as ready for review March 11, 2020 15:26
@ggalmazor ggalmazor requested a review from lognaturel March 11, 2020 15:26
@ggalmazor ggalmazor added this to the 1.17.3 milestone Mar 11, 2020
@ggalmazor
Copy link
Contributor Author

Since Davis has already tested it and we have a hard deadline on Friday, I'm expediting this PR into the needs testing category

Hoping y'all agree with me ;)

Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

I've confirmed that indeed BasicHttpEntity is not repeatable, that BufferedHttpEntity is repeatable when it wraps a BasicHttpEntity and that the code is clean.

@lognaturel
Copy link
Member

Works for me! @kkrawczyk123 is still busy with Collect regression testing but hopefully she can take an hour or so to check basic pushes to Central and Aggregate and see if she thinks of anything else that should be verified.

@ggalmazor
Copy link
Contributor Author

@DavisRayM, am I correct saying that the production servers don't have the 401 empty content patch yet? We're going to use the stage-odk instance to test this PR. Is that OK?

@DavisRayM
Copy link

DavisRayM commented Mar 13, 2020

@DavisRayM, am I correct saying that the production servers don't have the 401 empty content patch yet? We're going to use the stage-odk instance to test this PR. Is that OK?

Yes, you're correct @ggalmazor . I'll deploy the empty content fix to stage(currently not there)

@DavisRayM
Copy link

DavisRayM commented Mar 13, 2020

stage-odk.ona.io should be fine to use now 😄 @ggalmazor

@kkrawczyk123
Copy link
Contributor

@DavisRayM I am testing the pr, Did you use any specific form when you have encountered that issue? I am having some fails when pushing to ona server different forms - there might be some limitations ex. form versioning, specifying kind of media etc that do not occur for Central and Aggregate and you are aware of them but I am not.

@DavisRayM
Copy link

DavisRayM commented Mar 13, 2020

@DavisRayM I am testing the pr, Did you use any specific form when you have encountered that issue? I am having some fails when pushing to ona server different forms - there might be some limitations ex. form versioning, specifying kind of media etc that do not occur for Central and Aggregate and you are aware of them but I am not.

I didn't use any specific form while testing this.... Though @ggalmazor did report 404 errors popping up, these errors pop up in Ona due to the form version specified in the submission data being different from the version stored in Ona.

I might have answered this in a roundabout way ? Please let me know if this is unclear

In order to stop this from happening you might want to import data for forms that don't yet exist in your Ona stage account.

Making sure the version is the same on Ona and the submission data also helps.

@kkrawczyk123
Copy link
Contributor

@DavisRayM Are you able to push any form with media files to ona server?

@DavisRayM
Copy link

@kkrawczyk123 No not able to do so. Testing this out on different version of briefcase Don't want to block this PR if it's an Ona issue

@DavisRayM
Copy link

The 404 error seems to be consistent on the stage server even on the last stable version (atleast against Ona) v1.15.0 I'm assuming this is an issue on Ona's side. Most likely something to do with the version in the submission, sadly I can't research this at the moment...

I'd suggest not using Ona as the benchmark for now....

@kkrawczyk123
Copy link
Contributor

@ggalmazor @lognaturel as mentioned above I have encountered some issues when pushing forms to ona. They are connected to ona limitations like: ( forms/submissions with media are failing, form version differences forms with audits - I was not investigating all/other) not to briefcase and pr. I was able to push some forms to ona so I confirmed that it works for simple forms. I have also try pushing to Aggregate and Centrral all the ssme forms as for ona (also failing ones) and I haven't noticed any regression. I've checked cmd push too and it worked fine. If you have any other ideas what else should be verified please let me know. I was testing Ubuntu only.

@lognaturel
Copy link
Member

Thanks, Kasia! That seems sufficient to me.

@kkrawczyk123
Copy link
Contributor

@opendatakit-bot label "behavior verified"

@ggalmazor ggalmazor merged commit 9bffa68 into getodk:master Mar 13, 2020
@ggalmazor ggalmazor deleted the repeatable_http_requests branch March 13, 2020 14:32
elduwa pushed a commit to sw-testing-lab-briefcase/briefcase that referenced this pull request Mar 22, 2020
Marinolino added a commit to sw-testing-lab-briefcase/briefcase that referenced this pull request May 29, 2020
Marinolino added a commit to sw-testing-lab-briefcase/briefcase that referenced this pull request May 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to push forms to an aggregate server
5 participants