Feature/is expired#58 #59

OGKevin · 2017-10-12T19:48:52Z

Context

What has been done

This pr introduces a new way to handle SessionContext. Now it is possible to do the following:

if (!$apiContext->isSessionActive()) {
    $apiContext->resetSession();
    $apiContext->save();
}

This way there is no saving an unchanged context.

Closes #58

DennisSnijder

Simple yet effective PR!
Nice one 😄

DennisSnijder · 2017-10-12T19:56:19Z

src/Context/ApiContext.php

+     *
+     * @return bool
+     */
+    public function isExpired()


isExpired(): bool <- strict return type 😄

Aahhh good old strict type 😁 thx

DennisSnijder · 2017-10-12T19:59:37Z

src/Context/ApiContext.php


+    /**
+     * Checks if the session has expired


Perhaps rename this function to something like isSessionExpired which makes this comment unnecessary.

🤔 Fair enough will make this change in all the SDK's then 👍

dnl-blkv

A few comments :)

dnl-blkv · 2017-10-13T04:42:38Z

src/Context/ApiContext.php

@@ -273,15 +273,19 @@ private function dropSessionContext()
     */
    public function ensureSessionActive()
    {
-        if (is_null($this->sessionContext)) {
-            return;
+        if (!is_null($this->sessionContext) && $this->isSessionExpired()) {


check for is_null($this->sessionContext) can go from here, because you do it in isSessionExpired already :)

If you follow my suggestion and change isSessionExpired to isSessionActive or hasActiveSession, Please do not forget to add an exclamation mark here (only reset if NOT has an active session :))

Mhmm AFAIK if we do this then if session is null it will run in an infinite loop resetting the session. At least that was my expiernce in Java 🤔 will double check to make sure.

Ah yea! The infinite loop:

Maximum function nesting level of '256' reached, aborting!

And it also fails because:

$timeExpiry = $this->sessionContext->getExpiryTime()->getTimestamp();

This will cause an error as of sessionContext is null! So therefore we need !is_null($this->sessionContext) in ensureSessionActive still 🤔. The null sessionContext will get handled higher up the code before a request is made.

Ah but then based on your suggestion below this can be fixed 👍

dnl-blkv · 2017-10-13T04:46:11Z

src/Context/ApiContext.php

-        if ($timeExpiry - time() < self::TIME_TO_SESSION_EXPIRY_MINIMUM_SECONDS) {
-            $this->resetSession();
-        }
+        return is_null($this->sessionContext) || $timeExpiry - time() < self::TIME_TO_SESSION_EXPIRY_MINIMUM_SECONDS;


Since you do is_null($this->sessionContext) check here, the method can be be opposite to what we've had and be called isSessionActive or hasActiveSession. This also fits well into our ensureSessionActive :)

Also, I'd go for an explanatory variable here. I missed it when initially built ensureSessionActive %).

So something like this:

if (is_null($this->sessionContext)) { return false; } else { $timeExpiry = $this->sessionContext->getExpiryTime()->getTimestamp(); $timeToExpiry = $timeExpiry - time(); return self::TIME_TO_SESSION_EXPIRY_MINIMUM_SECONDS < $timeToExpiry; }

dnl-blkv · 2017-10-13T04:49:49Z

For the changes in my review: let's have this one merged, and then distribute the approach over the other SDKs :)

OGKevin · 2017-10-13T11:19:19Z

@dnl-blkv As we discussed, this pr is not ready. Fixing the issue in client that is causing the infinite loop due to client also resetting session. This must be prevent for endpoints:session-server, device-server and installation.

OGKevin · 2017-10-14T07:29:18Z

@dnl-blkv All yours 👀

dnl-blkv

A few comments %)

dnl-blkv · 2017-10-17T07:30:59Z

src/Http/ApiClient.php

 use Psr\Http\Message\ResponseInterface;

 /**
 */
 class ApiClient
 {
    /**
+     * These are the endpoints where there is no need to have an active session for the request


Please go for Endpoints not requiring active session for the request to succeed (less letters :P)

dnl-blkv · 2017-10-17T07:31:03Z

src/Http/ApiClient.php

+    const INSTALLATION_URL = "installation";
+
+    const URLS_TO_NOT_ENSURE_ACTIVE_SESSION = [
+        self::INSTALLATION_URL,


Please make it an associative array:

const URLS_TO_NOT_ENSURE_ACTIVE_SESSION = [ self::INSTALLATION_URL => true,

dnl-blkv · 2017-10-17T07:31:17Z

src/Http/ApiClient.php

+     * These are the endpoints where there is no need to have an active session for the request
+     * to succeed.
+     */
+    const SESSION_SERVER_URL = "session-server";


Please use single quotes for the constants!

Also please post the URIS_NOT_REQUIRING_ACTIVE_SESSION right after the doc header, and the constants right after it :)

Java struggles 😁

dnl-blkv · 2017-10-17T07:33:10Z

src/Http/ApiClient.php

    {
-        $this->apiContext->ensureSessionActive();
+        if (!in_array($uri, self::URLS_TO_NOT_ENSURE_ACTIVE_SESSION, true)) {


Once you have an associative array instead of a list, you can use isset(self::URLS_TO_NOT_ENSURE_ACTIVE_SESSION[$uri])

dnl-blkv · 2017-10-17T07:34:28Z

src/Http/ApiClient.php

+    const DEVICE_SERVER_URL = "device-server";
+    const INSTALLATION_URL = "installation";
+
+    const URLS_TO_NOT_ENSURE_ACTIVE_SESSION = [


Let's say URIS_NOT_REQUIRING_ACTIVE_SESSION for the sake of consistency :)

dnl-blkv · 2017-10-18T09:04:56Z

@andrederoos

OGKevin added 2 commits October 12, 2017 21:44

Added isExpired() method #58.

718f79b

Merge branch 'develop' into feature/isExpired#58

24387ab

OGKevin added the enhancement label Oct 12, 2017

OGKevin self-assigned this Oct 12, 2017

OGKevin requested a review from dnl-blkv October 12, 2017 19:48

DennisSnijder suggested changes Oct 12, 2017

View reviewed changes

Some cleaning 🔥 #58

18d62ca

dnl-blkv suggested changes Oct 13, 2017

View reviewed changes

OGKevin added 2 commits October 13, 2017 10:45

Removed double null check 👏 #58.

e2da44c

Renamed to isSessionActive() 😏 #58.

4b30d69

Fixed the issue where client will cause infinite loop 👏 #58.

80a3a70

dnl-blkv suggested changes Oct 17, 2017

View reviewed changes

OGKevin and others added 2 commits October 18, 2017 09:07

Isset 🔥

91f95fb

Tiny styling cleanup

552ef46

dnl-blkv approved these changes Oct 18, 2017

View reviewed changes

dnl-blkv merged commit 816f0fa into develop Oct 18, 2017

dnl-blkv deleted the feature/isExpired#58 branch October 18, 2017 09:04

OGKevin added this to the 0.12.2 milestone Oct 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/is expired#58 #59

Feature/is expired#58 #59

OGKevin commented Oct 12, 2017 •

edited

DennisSnijder left a comment

DennisSnijder Oct 12, 2017

OGKevin Oct 12, 2017

DennisSnijder Oct 12, 2017

OGKevin Oct 12, 2017

dnl-blkv left a comment

dnl-blkv Oct 13, 2017

dnl-blkv Oct 13, 2017

OGKevin Oct 13, 2017 •

edited

OGKevin Oct 13, 2017 •

edited

OGKevin Oct 13, 2017

dnl-blkv Oct 13, 2017

OGKevin Oct 13, 2017

dnl-blkv commented Oct 13, 2017

OGKevin commented Oct 13, 2017

OGKevin commented Oct 14, 2017

dnl-blkv left a comment

dnl-blkv Oct 17, 2017

dnl-blkv Oct 17, 2017

OGKevin Oct 17, 2017

dnl-blkv Oct 17, 2017

dnl-blkv Oct 17, 2017

OGKevin Oct 17, 2017

dnl-blkv Oct 17, 2017

OGKevin Oct 17, 2017

dnl-blkv Oct 17, 2017

dnl-blkv commented Oct 18, 2017

Feature/is expired#58 #59

Feature/is expired#58 #59

Conversation

OGKevin commented Oct 12, 2017 • edited

Context

What has been done

DennisSnijder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnl-blkv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin Oct 13, 2017 • edited

Choose a reason for hiding this comment

OGKevin Oct 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnl-blkv commented Oct 13, 2017

OGKevin commented Oct 13, 2017

OGKevin commented Oct 14, 2017

dnl-blkv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnl-blkv commented Oct 18, 2017

OGKevin commented Oct 12, 2017 •

edited

OGKevin Oct 13, 2017 •

edited

OGKevin Oct 13, 2017 •

edited