Make requestDate a parameter for presignedUrl

[dustin-H]

Hi there,

I'm using presigned urls pretty much in my projects. However, one problem is, that a client is reloading all images when it gets new urls. It is not cacheable by the browser as my server always responds new presigned urls with every request.

A solution would be to respond the same signed url within e.g. one day. So the client wouldn't get a new url with each and every request but instead one per day as it is cached.

However, a cache would need to store a lot of presigned urls, which I would like to avoid.

So I came to the idea of issuing all presigned urls to the beginning of the day. For example regardless of whether a file is requested at 9AM or 9PM, the server would respond a presigned url issued at 12AM (9 or 21 hours before). The signature would expire for example after 48 hours. This would respond the same presigned url on one day with every request.

To implement this, I need to set the requestDate in https://github.com/minio/minio-js/blob/master/src/main/minio.js#L1562.

My suggestion would be to add an optional parameter (requestDate) to the functions presignedUrl(), presignedGetObject() and presignedPutObject().

What do you think about that?

If you agree, I would create a PR.

Cheers, Dustin

[kannappanr]

@dustin-H You can use either Cache-Controle or Expires header as explained in https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectPUT.html .

[dustin-H]

Hi @kannappanr,

thanks, for the quick reply.

When I set a header on the url a cache would save the file for a certain time period.

However, this depends on the url including all GET-params. This will not work if a file has two signed urls issued at different dates.

Example:
I request a some data including some files at 9:00 AM.
I'll get a JSON including some presigned urls.

One of them could look like this: (simplified)

https://my-app.s3-eu-central-1.amazonaws.com/myImage.png?X-Amz-Date=20181023T090000Z&X-Amz-Expires=43200&X-Amz-Signature=1111

Then I request the same data again at 9:01 AM, because something has updated (but not the image).

I'll get the same image as presigned url:

https://my-app.s3-eu-central-1.amazonaws.com/myImage.png?X-Amz-Date=20181023T090100Z&X-Amz-Expires=43200&X-Amz-Signature=2222

Since the GET-parameters X-Amz-Date and thereby X-Amz-Signature have changed between those requests, the browser will treat this as different urls/files and will not respond the cached file from the first request for the second request.

The caching of a browser can only be achieved by issuing the signature to the same X-Amz-Date or requestDate, which will result in the exact same url.

Cheers, Dustin

[krishnasrinivas]

Since the GET-parameters X-Amz-Date and thereby X-Amz-Signature have changed between those requests, the browser will treat this as different urls/files and will not respond the cached file from the first request for the second request.

The caching of a browser can only be achieved by issuing the signature to the same X-Amz-Date or requestDate, which will result in the exact same url.

👍

My suggestion would be to add an optional parameter (requestDate) to the functions presignedUrl(), presignedGetObject() and presignedPutObject().

What do you think about that?

If you agree, I would create a PR.

This will be useful for everyone, please send the PR, thank you.

[dustin-H]

Hi @krishnasrinivas,

This will be useful for everyone, please send the PR, thank you.

Thanks! I just created the PR: #728

[satanTime]

Because presignedUrl is public method this change introduced breaking change due to changes in method's signature.
In my case, overrides on top of presignedUrl doesn't work after upgrade.
Please consider in future to release breaking changes with major version change, not minor.
Thanks.

[dustin-H]

Hi @satanTime

I don't understand how this breaks your your stuff.

The change was an additional and optional parameter. As the method is checking for function-types (callback-method) the old signature should still work well. This is why a minor release is totally fine for me.

[satanTime]

Hi @dustin-H,

I think that's the reason why you broke it :)

In computer science any breaking change of public interface is one of the most horrible things, that's why people have minor and major versions and support old interfaces to keep backward compatibility for some period. That's why we also have LTS versions etc.

The task you solved here could be done by extending minio client, instead of to change its source. And basically people, who're interested to have cached urls, could use new MinioRequestedDate client instead of Client. More about that you can read in SOLID principles.

In our project we use exactly this approach. Imagine that you have class which extends Client class from minio library and overrides presignedUrl method. And now this method is called with another set of arguments. Nice job :)

In proper world your change should be next major version with warning about breaking unsupported change.

[dustin-H]

Why do you explain computer science within a discussion at GitHub?

You still have not delivered any example how this can break any Javascript Code.

Criticising others by letting them look stupid is not the fine way.

Btw.: This wasn't even my decision.

[satanTime]

I explain computer science within a discussion at GitHub because some people on GitHub don't know it.

I mentioned an example above when someone extends Client class and overrides base method.
You can find an example how to override the method below:

import { Client, ResultCallback } from 'minio';

export class MyClient extends Client
{
    async presignedUrl(
        httpMethod: string,
        bucketName: string,
        objectName: string,
        expiryOrCallback?: number | ResultCallback<string>,
        reqParamsOrCallback?: { [key: string]: any; } | ResultCallback<string>,
        possibleCallback?: ResultCallback<string>,
    ): Promise<string> {
       console.log('forget about callback, now it is date object', possibleCallback);
       return super.presignedUrl(httpMethod, bucketName, objectName);
    }
}

to trigger error:

myClient.presignedGetObject("someBucket", "someDoc", 60 * 60 * 60);

And check how it works with 7.0.1 and 7.0.5.

About criticism and stupid - you say that, not me.
I only explained how people should develop software in right way.
It's fine, that some people fail, that's how we all gather experience and knowledge.

[dustin-H]

Okey, now we are talking.

The overriding itself isn't the single problem here. If you would use presignedUrl() directly, everything would just work fine.

Root of this error is the use of the public method presignedUrl() inside presignedGetObject().

As Javascript-Developer I am not used to do OOP-Stuff like overriding that often. So I haven't even thought about the fact, that someone could extend this class and override some basic methods which are used within the super class.
Doing this adds some complexity to your code, which is not trivial to understand. Calling presignedGetObject() jumps between the super- and sub-class.

I think the person who released the version as minor change has also not seen this potential issue as this is kind of complex.

Maybe we should make presignedUrl() private or not use a public method inside another.

Anyway, sorry for breaking your code.

[satanTime]

Agree, there're a lot of possible solutions. I don't have problem with change itself, our code is already fixed. What I wanted to highlight here is breaking change under minor version. I hope now, everybody is aware about this and in future breaking changes will be processed and released in more smooth way.

[krishnasrinivas]

Interesting ... JS is a tricky language, If you see below, Go behaves as expected. We'll note this behavior for future minio-js releases.

krishna@escape:~/dev$ cat jsprogs/test.js
class baseclass {
  firstFunc() {
    console.log("baseclass firstFunc")
  }
  secondFunc() {
    this.firstFunc()
    console.log("baseclass secondFunc")
  }
}

class superclass extends baseclass {
  firstFunc() {
    console.log("superclass firstFunc")
  }
}

var obj = new superclass()

obj.secondFunc()
krishna@escape:~/dev$ node jsprogs/test.js
superclass firstFunc
baseclass secondFunc
krishna@escape:~/dev$ cat goprogs/inheritance.go
package main

import "fmt"

type baseclass struct{}

func (b baseclass) firstFunc() {
	fmt.Println("baseclass firstFunc")
}

func (b baseclass) secondFunc() {
	b.firstFunc()
	fmt.Println("baseclass secondFunc")
}

type superclass struct {
	baseclass
}

func (s superclass) firstFunc() {
	fmt.Println("superclass firstFunc")
}

func main() {
	obj := superclass{}
	obj.secondFunc()
}
krishna@escape:~/dev$ go run goprogs/inheritance.go
baseclass firstFunc
baseclass secondFunc
krishna@escape:~/dev$ 

[satanTime]

Hi @krishnasrinivas, now to reproduce the issue change in your go func (s superclass) firstFunc() to func (s superclass) firstFunc(something int), not sure it will compile or as expected. That's what happened with the change in this issue.

[krishnasrinivas]

@satanTime it works as expected (same result as before)

[satanTime]

ah, understood your example. Looks like goland isn't suitable here. In JS it's next: before change you should get "superclass firstFunc".
After change you should get "baseclass firstFunc".

[harshavardhana]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Inheritance_and_the_prototype_chain on a related note for further reading.