add bodyFile to response part of pair #893

ns3777k · 2020-01-25T19:23:45Z

Hey!
I'm working on #815 and decided to make a PR so you can tell me if I'm on the right track. Tests pass locally, not sure why they are not run on CI here.

I'll add new tests and update docs after successful review.

Thanks.

tommysitu · 2020-01-26T19:23:42Z

Good stuff! I'll review it this week.

tommysitu

It's a good try. Let me know if you need any help.

core/handlers/v2/simulation_views_v1.go

core/handlers/v2/simulation_views_v3.go

core/handlers/v2/simulation_views_v4.go

core/handlers/v2/simulation_views_v5.go

core/models/payload.go

core/modes/modes.go

ns3777k · 2020-02-01T19:30:44Z

@tommysitu thanks for review. only 1 question for now. Does making v6 require copy pasting v5, renaming every struct to v6 and replacing v5 with v6 in all tests and other files?

tommysitu · 2020-02-03T12:41:20Z

@tommysitu thanks for review. only 1 question for now. Does making v6 require copy pasting v5, renaming every struct to v6 and replacing v5 with v6 in all tests and other files?

pretty much. also you need to add a function to upgrade v5 to v6 in simulation_views_upgrade.go

ns3777k · 2020-02-03T14:22:26Z

@tommysitu ok, got it, but I'm curios :-) wouldn't it be much easier to maintain the project with only 1 schema and avoiding BC? And in case of BC, release new major version with a migration guide (or a tool to upgrade existing files)?

tommysitu · 2020-02-03T23:52:50Z

@tommysitu ok, got it, but I'm curios :-) wouldn't it be much easier to maintain the project with only 1 schema and avoiding BC? And in case of BC, release new major version with a migration guide (or a tool to upgrade existing files)?

It's a little bit extra work to do but we can ensure that existing users can benefit from hoverfly upgrades without breaking their tests and CI pipelines. It could spark lots of support issues which is what we try to avoid.

ns3777k · 2020-02-03T23:56:45Z

@tommysitu Ok :-) Here we go again.
Now, if bodyFile is not empty, it's read and recorded into body. bodyFile remains in the response for information.
In NewResponseDetailsFromResponse there's no error checking for now, I'll add it tomorrow.
Current tests pass.

tommysitu · 2020-02-04T00:01:16Z

Nice, will check it out soon

ns3777k · 2020-02-05T22:06:49Z

I've added error handling when bodyFile is missing.

ns3777k · 2020-02-12T19:24:59Z

@tommysitu any chance you can review it soon? so i'll get on docs and tests

tommysitu · 2020-02-13T17:22:36Z

@tommysitu any chance you can review it soon? so i'll get on docs and tests

this weekends for sure

core/handlers/v2/simulation_views_v5.go

core/import.go

core/handlers/v2/simulation_views_upgrade.go

core/handlers/v2/simulation_views_upgrade_test.go

tommysitu · 2020-02-16T11:18:25Z

@ns3777k Thank you for your patience. I've finally got a chance to test it thoroughly. Here is what I found:

If the body file failed to read, it will wipe the existing simulation, because of this line:
https://github.com/SpectoLabs/hoverfly/blob/master/core/handlers/v2/simulation_handler.go#L127
I think this is something I need to fix afterwards
I couldn't get it to read a file relative to the hoverfly binary or the imported simulation. I think what would be really useful is that the user provide a relative path to the simulation file.
When both body and bodyFile exists, Hoverfly should pick one or the other, and prints a warning to the user. I would suggest body field takes precedence in this case.

ns3777k · 2020-02-16T18:03:05Z

I have fixed the comments above.

Should I fix this? Can you describe the case in more details?
Hmm. Can you show me the failed use case? Relative paths are resolved to the cwd, not the binary. If hoverfly was started from another directory, then the cwd would be that another directory. Starting hoverfly with hoverctl does not set the cwd for hoverfly (https://github.com/SpectoLabs/hoverfly/blob/master/hoverctl/wrapper/hoverfly.go#L173). Will setting the cwd there solve the case?
I've added the warning and now body takes precedence over bodyFile. The only thing left here is export cmd 'cause it exports both bodyFile and body filled from file. What would be the right behavior?

tommysitu · 2020-02-17T21:42:28Z

Regarding to the comments:

feel free to fix it if you got some ideas. we shouldn't allow a failed import deleting the simulation that was storing in Hoverfly
Basically I couldn't get relative path working at all. I've tried putting the file in the same directory as hoverfly binary or the simulation file, or the cwd. None of that works.

In the issue #815, @BrainStone proposed that the relative path could be relative to the Simulation file. I think that's a good idea, as you can keep these body files side-by-side to the simulation where they can be found easily.

For example, you can store your simulation and body file with this structure:

.
+-- simulations 
|   +-- flight_api.json
|   +-- flight_api_responses
    |   +-- 200.json
    |   +-- 400.json

and the bodyFile in your flight_api.json can set to flight_api_response/200.json and flight_api_response/400.json regardless of where you start your hoverfly or use hoverctl.

Thanks for the PR. The fix looks good to me. No need to change the export cmd. As we have given user a warning, we'd expect them to fix the simulation and decide what they want to keep.

ns3777k · 2020-02-17T22:00:31Z

Ok, I'll think about it this week
Hmmm. Say I wanna use hoverctl import or API directly to send a pair with bodyFile in response. The simulation file is located locally (on client) but hoverfly server can be run on a different machine and hoverfly binary won't be able to find files relative to simulation file thay was uploaded from a client machine. Am I missing something here? 😕
Ok, cool

tommysitu · 2020-02-17T22:45:32Z

mmm. Say I wanna use hoverctl import or API directly to send a pair with bodyFile in response. The simulation file is located locally (on client) but hoverfly server can be run on a different machine and hoverfly binary won't be able to find files relative to simulation file that was uploaded from a client machine. Am I missing something here?

you're right, it won't work if hoverfly is remote. What about reading the bodyFile is done locally in core/import.go and hoverctl/cmd/import.go? I think that solves the first issue as well.

ns3777k · 2020-02-18T00:10:13Z

I've got confused a little 😃

So, first off is to patch hoverctl to loop through all pairs, replacing bodyFile with body filled with files' contents. Would be a little tricky 'cause we should unmarshal simulation data, but no problems so far.
Touches only client side, so no BC for server and no need for v6 and supporting bodyFile in schema, right?

Then patch hoverfly import cmd to do the same thing.

Question is: should I keep v6 with bodyFile in schema?

tommysitu · 2020-02-19T22:06:02Z

Not entirely sure if reading the bodyFile in the hoverctl import is the right way to go because:

It won't work if someone imports the simulation using the Admin API
Any tools that build around the hoverfly Admin API (such as Hoverfly-Java) have to replicate the logic to handle bodyFile.

sorry for contradicting to what I said 😳 . I underestimated the challenges of adding this feature.

So if we keep what you have done, then we must find a way to handle relative path. Any thoughts?

Nevertheless, we should keep the v6 schema.

tommysitu · 2020-02-19T22:31:21Z

Here's what I've tried:

make build
start hoverfly binary in the target folder

$ target/hoverfly

copy the body file to the same folder as the hoverfly binary:

$ cp 200.json ~/go/src/github.com/SpectoLabs/hoverfly/target/200.json

import the simulation that has "bodyFile": "200.json",, then it blows up:

$ hoverctl import time.json
Could not import simulation

{"error":"An error occurred: open 200.json: no such file or directory"}{"data":{"pairs":[],"globalActions":{"delays":[],"delaysLogNormal":[]}},"meta":{"schemaVersion":"v6","hoverflyVersion":"v1.1.3","timeExported":"2020-02-19T22:24:11Z"}}

ns3777k · 2020-02-19T22:34:04Z

that's correct because your cwd is not target, but the directory where target is at. in other words valid cwd = $PWD. try to put 200.json on the same level where target directory is.

tommysitu · 2020-02-19T22:58:42Z

ok, it's relative to where the hoverfly binary is executed.

ns3777k · 2020-02-19T22:59:19Z

It can find the file in Download because you probably run the hoverfly from the same directory? If you're on linux (not sure about mac) you run the following command to know the wd of the process: pwdx pgrep hoverfly.

That's usually $PWD unless changed.

ns3777k · 2020-02-19T23:46:27Z

race condition 😄 so, there are a couple things left here:

prevent deleting the simulation on failed import
write some tests
write some docs

right?

tommysitu · 2020-02-20T00:02:42Z

pretty much. If you can fix 1 that will be great. 🌟 feel free to do 2 and 3 in a separate PR

ns3777k · 2020-02-20T22:10:26Z

According to removal the simulation on a file error. I think I've found one easy way to prevent this.

Methods inside Hoverfly::DeleteSimulation should return the removed elements and get saved in the new backup simulation which is returned from Hoverfly::DeleteSimulation.
New method Hoverfly::RestoreSimulation or Hoverfly::ReplaceSimulation should be added to replace the current simulation with the previous backed up in case of error.

Pseudo code:

hoverfly_service.go:

before:

func (this *Hoverfly) DeleteSimulation() {
	this.Simulation.DeleteMatchingPairs()
	this.DeleteResponseDelays()
	this.DeleteResponseDelaysLogNormal()
	this.FlushCache()
}

after:

func (this *Hoverfly) RestoreSimulation(s *models.Simulation) {
	this.Simulation.DeleteMatchingPairs()
	this.Simulation.ResponseDelays = s.ResponseDelays
	this.Simulation.ResponseDelaysLogNormal = s.ResponseDelaysLogNormal
	pairs := s.GetMatchingPairs()
	this.Simulation.AddPairs(pairs)
}

func (this *Hoverfly) DeleteSimulation() *models.Simulation {
	pairs := this.Simulation.DeleteMatchingPairs()
	delays := this.DeleteResponseDelays()
	delaysLogNormal := this.DeleteResponseDelaysLogNormal()
	this.FlushCache()

	s := models.NewSimulation()
	s.ResponseDelays = d1
	s.ResponseDelaysLogNormal = d2
	s.AddPairs(pairs)

	return s
}

In simulation_handler.go we save the simulation returned from DeleteSimulation and pass it to RestoreSimulation in case of errors.

There is a thing I didn't figure out for now. From what I see, ResponseDelays and ResponseDelaysLogNormal can be written in and read from but they are not protected with a mutex.

tommysitu · 2020-02-24T22:19:15Z

Sorry for the late reply, I was under the weather. Just looked at your proposed solution, I'm a bit wary about the potential performance impact. We were aware that some hoverfly users have huge simulation files loaded in. It would be great if none of the existing hoverfly users gets much penalty for upgrading.

What about looping through the pair, and testing any presence of bodyFile to see if they are readable instead?

We already have simulationView, err := NewSimulationViewFromRequestBody(body) in the addSimulation, so using function such as os.OpenFile to test bodyFile should be relatively simple to do as well.

tommysitu · 2020-02-24T22:21:40Z

After thinking more about the relative path, we could make it a config option for user to specify where the root path it's relative to. That should solve the problem when user wants to store the file somewhere else rather than the cwd of hoverfly binary.

ns3777k · 2020-02-25T23:40:50Z

This sounds much simpler but I'm not sure about performance impact, 'cause os.Open and f.Close both make a syscall. Would this be better than delays sync?
Agree. I'll make this option to be cwd by default. Though I'm not sure for now how to pass this option into NewResponseDetailsFromResponse, but I'll take a look.

Actually, current ioutil.ReadFile already does os.Open, and other stuff. Maybe there's no need to open the same file twice?

tommysitu · 2020-02-26T00:16:45Z

ignore os.openfile, that was my brain fart. What about os.stat()? It only reads the file metadata. I can’t tell what the actual performance is without a benchmark. I’m suggesting that we should do something to avoid impacting existing users when they upgrade. Alternatively It can do the backup only when body file is used.

ns3777k · 2020-02-26T17:26:48Z

os.stat does not check whether a file is able to be read by the process (e.g. permissions) it can only tell that a file exists. i'll dig deeper soon :-)

tommysitu · 2020-02-26T17:30:29Z

Thanks for looking into this!

…iles before dropping simulation

ns3777k · 2020-02-29T11:30:45Z

So, that's what I've come up with (latest commit):

We can avoid additional performance impact if we read the bodyFiles into body right after creating a simulation view (we'll read them anyway later). So we can early return on error before deleting simulations.

I moved the body reading part into PutSimulation method as well as deleting simulation call because I didn't want to read body files inside context of http handler. So basically I encapsulated this piece of logic there so everything is in 1 place.

I've added the configuration option -response-body-files-path, but I suck at naming things 😄 maybe you've gotta better one.

ns3777k · 2020-03-04T21:40:12Z

what's the plan? 🕵️‍♂️ should i start on the docs and tests? or you need some time to review this (last commit should solve previous issues)? should i make a separate pr for tests and docs?

tommysitu · 2020-03-04T21:50:04Z

I need to do some more testing around your last commit. How urgently do you need this feature? I'm planning to a bug fix release for v1.1.5 this week. Then this one can go into the next one v1.2.0 with another two issues that need schema changes:
#807
#761

What do you think?

ns3777k · 2020-03-04T21:57:38Z

No urgency. Just wanted to know the plan 😄 sounds cool

tommysitu · 2020-03-04T22:00:16Z

We will get there. I'll find more spare time on this and getting v1.2 out in the next couple of weeks. 💪

tommysitu

I've tested it, it's working really well, especially you can load an image file without storing it in the simulation. Just nitpicking over some tiny details

core/models/payload.go

tommysitu · 2020-03-07T23:59:46Z

core/cmd/hoverfly/main.go

+	dir, err := os.Getwd()
+	if err != nil {
+		fmt.Println(err)
+		os.Exit(1)
+	}
+


Is this needed? I thought if the root path is empty, the default resolving location will be the cwd?

Yes 'cause getting the cwd can return an error (e.g. http://man7.org/linux/man-pages/man2/getcwd.2.html).

But I'll double check if we can ignore the error and keep the dir variable to be an empty string. Thus we just delay the error handling and it'll occur when a bodyFile is not able to be read from in case it contains a relative path.

what I meant is:

ioutil.ReadFile(filepath.Join("", pair.Response.GetBodyFile()))

should read the file under the current working directory, right?
so setting the response-body-files-path to "" should be equivalent to setting it to os.Getwd()?

Oh, yes. I used the os.Getwd to show the current dir in --help information, otherwise there will be an empty string there. Should I remove it?

core/hoverfly_service.go

tommysitu · 2020-03-08T00:26:04Z

Also I realized one more thing: you've loaded the file and put it in the body field. so it will show up in the exported simulation. Can we do something about this? Perhaps adding some way to keep track if the body was from the bodyFile and should be excluded on export.

ns3777k · 2020-03-08T01:04:14Z

Yes, that's exactly what I asked some days ago #893 (comment) 😄 I'll think what can be done to avoid this.

tommysitu · 2020-03-08T22:28:20Z

This PR is on the right track, I quite like to merge it to the next release branch (https://github.com/SpectoLabs/hoverfly/tree/v1.2.0). Do you mind reopen this issue for merging into the v1.2.0 branch? Then we can tackle the export issue. Thanks!

tommysitu · 2020-03-08T22:29:29Z

also it's quite a long thread, that's probably why I was losing track 😆

ns3777k · 2020-03-08T22:57:56Z

I've switched the base branch to v1.2.0

ns3777k added 2 commits January 25, 2020 21:29

add bodyFile to response part of pair

976fce0

fix comments

df04f0f

ns3777k changed the title ~~WIP: add bodyFile to response part of pair~~ add bodyFile to response part of pair Jan 25, 2020

tommysitu requested changes Jan 30, 2020

View reviewed changes

ns3777k added 2 commits February 3, 2020 23:42

v6 all files

65e28e1

schema upgrade v5 => v6

3de3a68

tommysitu force-pushed the master branch from f0e3091 to 27b6606 Compare February 3, 2020 22:40

read file on upload

f2c17c0

add error handling when bodyFile is missing

60f9e8e

tommysitu reviewed Feb 16, 2020

View reviewed changes

core/handlers/v2/simulation_views_v5.go Outdated Show resolved Hide resolved

core/import.go Outdated Show resolved Hide resolved

tommysitu reviewed Feb 16, 2020

View reviewed changes

core/handlers/v2/simulation_views_upgrade.go Show resolved Hide resolved

core/handlers/v2/simulation_views_upgrade_test.go Show resolved Hide resolved

cr fixes

3d79b02

body and bodyFile at the same time

4a482d4

tommysitu approved these changes Feb 20, 2020

View reviewed changes

add response body files relative root path cfg option and read body f…

945a074

…iles before dropping simulation

tommysitu reviewed Mar 8, 2020

View reviewed changes

fix cr

70ae5cd

ns3777k changed the base branch from master to v1.2.0 March 8, 2020 22:57

tommysitu merged commit 1750611 into SpectoLabs:v1.2.0 Mar 8, 2020

ns3777k deleted the body-file branch March 8, 2020 23:01

add bodyFile to response part of pair #893

add bodyFile to response part of pair #893

Conversation

ns3777k commented Jan 25, 2020 • edited Loading

tommysitu commented Jan 26, 2020

tommysitu left a comment • edited Loading

Choose a reason for hiding this comment

ns3777k commented Feb 1, 2020

tommysitu commented Feb 3, 2020

ns3777k commented Feb 3, 2020 • edited Loading

tommysitu commented Feb 3, 2020 • edited Loading

ns3777k commented Feb 3, 2020 • edited Loading

tommysitu commented Feb 4, 2020 • edited Loading

ns3777k commented Feb 5, 2020

ns3777k commented Feb 12, 2020 • edited Loading

tommysitu commented Feb 13, 2020

tommysitu commented Feb 16, 2020 • edited Loading

ns3777k commented Feb 16, 2020 • edited Loading

tommysitu commented Feb 17, 2020

ns3777k commented Feb 17, 2020 • edited Loading

tommysitu commented Feb 17, 2020

ns3777k commented Feb 18, 2020 • edited Loading

tommysitu commented Feb 19, 2020 • edited Loading

tommysitu commented Feb 19, 2020

ns3777k commented Feb 19, 2020 • edited Loading

tommysitu commented Feb 19, 2020

ns3777k commented Feb 19, 2020 • edited Loading

ns3777k commented Feb 19, 2020

tommysitu commented Feb 20, 2020 • edited Loading

ns3777k commented Feb 20, 2020

tommysitu commented Feb 24, 2020

tommysitu commented Feb 24, 2020 • edited Loading

ns3777k commented Feb 25, 2020 • edited Loading

tommysitu commented Feb 26, 2020 • edited Loading

ns3777k commented Feb 26, 2020

tommysitu commented Feb 26, 2020

ns3777k commented Feb 29, 2020

ns3777k commented Mar 4, 2020 • edited Loading

tommysitu commented Mar 4, 2020

ns3777k commented Mar 4, 2020 • edited Loading

tommysitu commented Mar 4, 2020

tommysitu left a comment • edited Loading

Choose a reason for hiding this comment

tommysitu Mar 7, 2020

Choose a reason for hiding this comment

ns3777k Mar 8, 2020 • edited Loading

Choose a reason for hiding this comment

ns3777k Mar 8, 2020

Choose a reason for hiding this comment

tommysitu Mar 8, 2020

Choose a reason for hiding this comment

ns3777k Mar 8, 2020 • edited Loading

Choose a reason for hiding this comment

tommysitu commented Mar 8, 2020

ns3777k commented Mar 8, 2020

tommysitu commented Mar 8, 2020

tommysitu commented Mar 8, 2020

ns3777k commented Mar 8, 2020 • edited Loading

ns3777k commented Jan 25, 2020 •

edited

Loading

tommysitu left a comment •

edited

Loading

ns3777k commented Feb 3, 2020 •

edited

Loading

tommysitu commented Feb 3, 2020 •

edited

Loading

ns3777k commented Feb 3, 2020 •

edited

Loading

tommysitu commented Feb 4, 2020 •

edited

Loading

ns3777k commented Feb 12, 2020 •

edited

Loading

tommysitu commented Feb 16, 2020 •

edited

Loading

ns3777k commented Feb 16, 2020 •

edited

Loading

ns3777k commented Feb 17, 2020 •

edited

Loading

ns3777k commented Feb 18, 2020 •

edited

Loading

tommysitu commented Feb 19, 2020 •

edited

Loading

ns3777k commented Feb 19, 2020 •

edited

Loading

ns3777k commented Feb 19, 2020 •

edited

Loading

tommysitu commented Feb 20, 2020 •

edited

Loading

tommysitu commented Feb 24, 2020 •

edited

Loading

ns3777k commented Feb 25, 2020 •

edited

Loading

tommysitu commented Feb 26, 2020 •

edited

Loading

ns3777k commented Mar 4, 2020 •

edited

Loading

ns3777k commented Mar 4, 2020 •

edited

Loading

tommysitu left a comment •

edited

Loading

ns3777k Mar 8, 2020 •

edited

Loading

ns3777k Mar 8, 2020 •

edited

Loading

ns3777k commented Mar 8, 2020 •

edited

Loading