TK2d #508
TK2d #508
Conversation
| @@ -14,10 +14,16 @@ | |||
|
|
|||
| ART_ASSUME_NONNULL_BEGIN | |||
|
|
|||
| @protocol ARTDefaultTokenParams <NSObject> | |||
There was a problem hiding this comment.
Hmm, I don't think this is a sound solution. This isn't really buying us anything, as we can in fact set defaultTokenParams to an ARTTokenParams with timestamp, and then we just force-convert defaultTokenParams back to ARTTokenParams (here), so the timestamp would still be there! (Plus, that's generally a type-unsafe conversion.)
I don't think it really pays off to introduce a new type for this; you can just check on defaultTokenParams's setter that timestamp is nil.
But if you want to introduce a new type to ensure this at compile time, I think ARTDefaultTokenParams shouldn't be a protocol but a class, with a public interface just like ARTTokenParams but without a timestamp property. That would ensure that we don't have a timestamp there, of course. It should have a method toTokenParams to be used in the line above that returns an ARTTokenParams with a nil timestamp. (Implementation suggestion: ARTDefaultTokenParams could have a ARTTokenParams private property or instance variable to which it would just delegate all calls, and then return it when toTokenParams is called.)
There was a problem hiding this comment.
You're right, the protocol is not a good solution. You can access the timestamp with a simple hack performSelector("getTimestamp").
Initially I was thinking to just assign nil to the timestamp on the defaultTokenParams setter but then I faced failures with this spec. Now I was looking to the 0.8 specs and I can't find anything related with TokenParams .timestamp should be generated at the getter and stick.
|
@tcard PTAL |
| let params = ARTTokenParams() | ||
|
|
||
| waitUntil(timeout: testTimeout) { done in | ||
| let now = NSDate().artToIntegerMs() | ||
| guard let timestamp = params.timestamp else { |
There was a problem hiding this comment.
I am not sure I follow this. If a user creates a TokenParams object, if I am reading this correctly, you are setting a timestamp as part of the token params. So if the user then did something async, and later did an auth using the TokenParams, the timestamp woudl be the timestamp the TokenParams object was created? If so, that is wrong. The vanilla TokenParams object has no timestamp value unless explicitly set. Then when a token request is issued, if the TokenParams object has a value it uses it, if not the current timestamp is used. There is no benefit in issuing a timestamp when the TokenParams object is initialized whatsoever, it will simply mean that it's more likely that by the time a token request is created, the timestamp will be out of date.
There was a problem hiding this comment.
@mattheworiordan Yes, I changed the TokenParams initialiser to always include a timestamp.
TokenParams object has no timestamp value unless explicitly set
I will fix it.
There was a problem hiding this comment.
Ok, glad we spotted this then :/
There was a problem hiding this comment.
Fixed 0624ee2.
|
Can I squash and rebase? |
No description provided.