ijpiantanida / talkback

A simple HTTP proxy that records and playbacks requests
MIT License
283 stars 41 forks source link

fix: implement workaround for issue [#91] #92

Closed raphaeleidus closed 3 months ago

raphaeleidus commented 4 months ago

Allow for the talkback server to be started with { numberedTapes: false } to get safer tape storage that is still in order of tape creation.

Fix for #91

ijpiantanida commented 3 months ago

Thanks for the PR and for submitting the issue in the first place!

I would like to avoid having to add a new option for this to keep it simple. If the update sticks to using an numeric version number, it should be a fairly minor API change and should have relatively low impact. I'm not even sure it warrants a major version update.

I like the idea of using the timestamp, although we might need more than milliseconds precision, since 2 requests could in theory be created on the same millis. If we can keep track of the last issued "tape id", then we can check if the one we are about to issue (the real millis epoch) is <= to that one, and artificially bump the last one by 1 (I don't think you'd be able to have several requests in the same milli to the point where this mechanism is a problem).

I suspect a change like this will break a few tests as they currently rely on the "current tape id" to find new tapes created by the test.

Happy to take it from here!

raphaeleidus commented 3 months ago

Yeah, there were a bunch of tradeoffs in possible approaches. I really didn't want to change the signature of tapeNameGenerator to take a string | number as that would be a breaking change.

In practice it is SUPER unlikely to get 2 tapes generated in the same millisecond, but it is possible.

I thought about trying to persist a lastTapeId to disk in a file, but then that has to be entered into version control and there is nothing really preventing user error and failing to commit that file. I thought about trying to parse the highest id out of the filename, but that is pretty much impossible while still allowing users to supply a tapeNameGenerator function

Ultimately in the project my team is working on we use this:

import crypto from 'crypto';
const tapeNameGenerator = (tapeNumber, tape) => {
    const shasum = crypto.createHash('sha1');
    shasum.update(JSON.stringify(tape.req));
    const digest = shasum.digest('hex');
    return digest.substring(0, 16);
}

A few nice things about this: ^ The names are hashes of the matching conditions which ensures uniqueness, theoretically allows for on the fly loading because given a request you can calculate where the response would be. We have 10 talkback instances running in different folders and this ends up with each filename being unique so it is really fast to just from console output of just a filename without a path, straight into the correct file.

Downsides: The files aren't in sequential order (minor), if the contents are hand modified, the relationship between the filename and the match conditions could be broken and lead to a file being overwritten. Creates a dependency on crypto.

raphaeleidus commented 3 months ago

to avoid the possibility of same millisecond tapes you can do:

const t = performance.now() + performance.timeOrigin;
const fileId = Math.round(t * 100).toString(36); // Precision to 10 microseconds 
ijpiantanida commented 3 months ago

@raphaeleidus I agree on all your points. What do you think of this approach? https://github.com/ijpiantanida/talkback/commit/3edd6131478349c3267c40cc20c94d96734d3d4c

raphaeleidus commented 3 months ago

@ijpiantanida I like it. Simple. I was trying to not make the file names super long with my base 36 encoding, but the complexity of going from numbers to strings isn't worth it. I like your approach.

ijpiantanida commented 3 months ago

Thanks @raphaeleidus - this has been released in 4.1.0