Skip to content

Conversation

dcsg
Copy link
Contributor

@dcsg dcsg commented Sep 27, 2020

In order to use the single table pattern of DynamoDB it would be nice to customize the separator to fit different use cases. This PR adds that possibility by allowing to define that option.

@Nyholm
Copy link
Member

Nyholm commented Sep 27, 2020

Thank you for this. Just a small question: when you say “single table pattern”, you you mean multiple applications that share the same table?
What I’m thinking is: do we need a “prefix” more than a “separator”?

@dcsg
Copy link
Contributor Author

dcsg commented Sep 27, 2020

you you mean multiple applications that share the same table?

yes, that's the use case.

What I’m thinking is: do we need a “prefix” more than a “separator”?

Well, I do define the session name in both application in which I'm assuming it is the prefix. By that I mean in one application I can have session name to be A and in another application to be B and then in dynamo I might have a record A#1234 and another B#1234, etc. How would you define the prefix?

@jderusse
Copy link
Member

I'm not sure to understand what problem does it solve.

@dcsg
Copy link
Contributor Author

dcsg commented Sep 27, 2020

@jderusse it solves the problem that I can define my own separator to have multiple sessions from multiple application in a single table. Because the code does not allow me to extend this to change the separator.

image

Line 2 is what I want to achieve line 4 is what I get with the current code (replace 3cket with BACKOFFICE).

The usual separator when using single table design/pattern is the # not the _.

@jderusse
Copy link
Member

using separator to namespace application sounds hacky. You should use a prefix for this as suggested by @Nyholm .

But, I don't think it's necessary. Each application should have its own session is, you shouldn't have any conflict.
Look at standard php session, or memcached/redis handler, does they provide such behavior? Isn't the purpose of sessionName?

If the issue is about # vs _ to respect a common standard in DynamoDb, this PR should be about changing the separator, and not about making it parameterizable.

@dcsg
Copy link
Contributor Author

dcsg commented Sep 27, 2020

Look at standard php session, or memcached/redis handler, does they provide such behavior?

TBH I didn't look into it.

Isn't the purpose of sessionName?

Defining the sessionName is what for me means having a prefix, it can be achieved by having different session names for each application.

If the issue is about # vs _ to respect a common standard in DynamoDb, this PR should be about changing the separator, and not about making it parameterizable.

Well, thats not my decision. But be aware of that changing this to # if instead of making this parameterizable it will mean a BC break for those using this already. The question is, what do we lose making this something people can customise?

@dcsg
Copy link
Contributor Author

dcsg commented Sep 29, 2020

@Nyholm @jderusse any updates on this?

@Nyholm
Copy link
Member

Nyholm commented Sep 29, 2020

I think the _ is a bit magic. I dont fully understand why we ever would like to change the separator.

Since @dcsg clearly has a use case for it, then I dont mind merging this PR.

What Im currently dont know (and it may be work for a later PR) is if we should update our default or not.

@dcsg
Copy link
Contributor Author

dcsg commented Sep 29, 2020

The pattern I described is Overloading keys/indexes, you can see a lab here: https://amazon-dynamodb-labs.com/design-patterns/ex4gsioverload.html

There isn't a strict rule that say it must be # the separator. However the # is vastly used because it helps to identify it easier. One disavantage of using _ is that it is also used to separate words and it could be confusing. Ofc if you have a table only to store sessions this isn't an issue but if you use the single table pattern this can be very weird.

@dcsg
Copy link
Contributor Author

dcsg commented Sep 29, 2020

What Im currently dont know (and it may be work for a later PR) is if we should update our default or not.

I would say that probably most people don't use the Single Table Pattern so the separator for them does not matter much. But giving the possibility to customize it, would allow those that use the pattern to have the same separator for everything. Also since there isn't a strict rule for this (the separator), it means that others might choose a completly different character for the separator.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im happy with this PR.

We should consider using # as our default separator before 1.0

@jderusse
Copy link
Member

If the goal is to replace _ by #, then making it parameterizable is overkill. Just change the default value, people will lost their session, I don't think its a blocker.

If the purpose is to namespace app (let's say App A use |, App B use ~), this is not the right way to go.

Unless I missed another use case, I'm 👎 for this PR.

@Nyholm
Copy link
Member

Nyholm commented Sep 29, 2020

@guillaumesmo do you remember if there were any specific reason you decided to use _?

@dcsg
Copy link
Contributor Author

dcsg commented Sep 29, 2020

If the purpose is to namespace app (let's say App A use |, App B use ~), this is not the right way to go.

Usually that's not what happens, when you use the Single Table Pattern you use the same separator for that table. However, that separator might be #, |, ~, this separator is common for the entire table regardless where the data comes from.

Having a separator might be overkill but in that case we can simple remove it and then it is simple has setting the session name with the separator you like, eg: session_name: 'MyApp#'. It would also make the code simpler, I'm ok with both approaches but I do see benefits in giving control to the users to define their separator either by being an option or by be included in the session name.

@guillaumesmo
Copy link
Contributor

guillaumesmo commented Sep 29, 2020

@guillaumesmo do you remember if there were any specific reason you decided to use _?

yes: https://github.com/aws/aws-sdk-php/blob/master/src/DynamoDb/SessionHandler.php#L233

edit to make it clear, idea was to keep 1:1 functionality with aws-sdk so that migration can be done without losing sessions

@jderusse
Copy link
Member

jderusse commented Oct 5, 2020

Well, I do define the session name in both application in which I'm assuming it is the prefix. By that I mean in one application I can have session name to be A and in another application to be B and then in dynamo I might have a record A#1234 and another B#1234, etc. How would you define the prefix?

Hello @dcsg , did you tried using session name in both App?

With symfony:

framework:
    session:
        name: A

If you don't use symfony, did you tried with https://www.php.net/manual/fr/function.session-name.php

session_name('A');

This is the simplest way to define a prefix.

1 similar comment
@jderusse
Copy link
Member

jderusse commented Oct 5, 2020

Well, I do define the session name in both application in which I'm assuming it is the prefix. By that I mean in one application I can have session name to be A and in another application to be B and then in dynamo I might have a record A#1234 and another B#1234, etc. How would you define the prefix?

Hello @dcsg , did you tried using session name in both App?

With symfony:

framework:
    session:
        name: A

If you don't use symfony, did you tried with https://www.php.net/manual/fr/function.session-name.php

session_name('A');

This is the simplest way to define a prefix.

@dcsg
Copy link
Contributor Author

dcsg commented Oct 5, 2020

Well, I do define the session name in both application in which I'm assuming it is the prefix. By that I mean in one application I can have session name to be A and in another application to be B and then in dynamo I might have a record A#1234 and another B#1234, etc. How would you define the prefix?

Hello @dcsg , did you tried using session name in both App?

With symfony:

framework:
    session:
        name: A

If you don't use symfony, did you tried with https://www.php.net/manual/fr/function.session-name.php

session_name('A');

This is the simplest way to define a prefix.

This is how I do define the prefix, but again I cannot control the separator.

@jderusse
Copy link
Member

jderusse commented Oct 5, 2020

why do you need to control the separator? What problem does it solve?

in this comment #792 (comment) you say "it solves the problem that I can define my own separator" it does not explain what is the original problem with BACKOFFICE_012345 and how BACKOFFICE#012345 would fix it.

@dcsg
Copy link
Contributor Author

dcsg commented Oct 5, 2020

why do you need to control the separator? What problem does it solve?

in this comment #792 (comment) you say "it solves the problem that I can define my own separator" it does not explain what is the original problem with BACKOFFICE_012345 and how BACKOFFICE#012345 would fix it.

I explained it. Usually when using single table pattern the separator is the # but it can be something else there isn't a strict rule regarding the separator but you must use the same separator when using that pattern and here its forcing always to be _.

@jderusse
Copy link
Member

jderusse commented Oct 5, 2020

you must use the same separator when using that pattern

You mean, you have 2 applications, that share the same table, and share the sames session data?

Both applications use the same prefix (in order to read the same data), both application are able to deserialize php session and extract data from _sf2_attributes?

Having _ as a separator in one application prevent the second application to read the session data

@dcsg
Copy link
Contributor Author

dcsg commented Oct 9, 2020

Instead of continue to explain my self again and again I will put this in this terms. Me as a user of this lib I want the ability to customize the separator. Saying this, what harm does it does by allowing it? And what cons exists to allow it?

@jderusse
Copy link
Member

jderusse commented Oct 9, 2020

are you storing anything other than sessions in the dynamoDb table?

@dcsg
Copy link
Contributor Author

dcsg commented Oct 9, 2020

are you storing anything other than sessions in the dynamoDb table?

yes

@jderusse
Copy link
Member

jderusse commented Oct 9, 2020

Ok, I finally get what you are trying to achieve

@jderusse jderusse merged commit c43127d into async-aws:master Oct 9, 2020
@jderusse
Copy link
Member

jderusse commented Oct 9, 2020

Thank you @dcsg

@Nyholm
Copy link
Member

Nyholm commented Oct 9, 2020

I must admit that I didn't really understand "single table pattern" either.

Thank you @dcsg for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants