Add JsonFileSessionStore #18

Laiteux · 2020-09-14T02:31:56Z

Laiteux · 2020-09-14T02:33:31Z

Let me know what you think.

Laiteux · 2020-09-14T02:42:54Z

Note that you have to add Newtonsoft.Json as a reference to the startup project (the one you're using TgSharp with), however this error occurs:

Exception levée : 'System.IO.FileNotFoundException' dans Newtonsoft.Json.dll
Exception levée : 'System.IO.FileNotFoundException' dans System.Private.CoreLib.dll
Exception levée : 'System.IO.FileNotFoundException' dans System.Private.CoreLib.dll
Une exception de type 'System.IO.FileNotFoundException' s'est produite dans System.Private.CoreLib.dll mais n'a pas été gérée dans le code utilisateur
Could not load file or assembly 'System.Security.Permissions, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. Le fichier spécifié est introuvable.

This might be annoying. After a few Google searches, I tend to believe the issue is due to the fact we recently migrated to .NET Standard 2.0 or .NET Core: https://stackoverflow.com/a/48043024/12753809

knocte · 2020-09-14T06:39:43Z

we recently migrated to .NET Standard 2.0 or .NET Core

who is 'we'? I don't understand this sentence, can you clarify

Laiteux · 2020-09-14T07:01:11Z

we recently migrated to .NET Standard 2.0 or .NET Core

who is 'we'? I don't understand this sentence, can you clarify

"we" = TgSharp. Nevermind tho, I used the master branch and it wasn't migrated to .NET Standard 2.0 yet. Forget about that.

knocte · 2020-09-14T07:07:57Z

it wasn't migrated to .NET Standard 2.0 yet. Forget about that.

So you mean the System.IO.FileNotFoundException problem will only happen when we change TgSharp to .NETStandard?

Laiteux · 2020-09-14T07:09:49Z

So you mean the System.IO.FileNotFoundException problem will only happen when we change TgSharp to .NETStandard?

No, it currently is happening. I thought that was because we migrated to .NET Standard but I was wrong. Maybe migrating will fix the issue?

knocte · 2020-09-14T07:10:58Z

I see, ok.

BTW you haven't said why is this JsonFileSessionStore useful at all? Why did you include it? What benefit does it bring to you?

Laiteux · 2020-09-14T07:14:44Z

BTW you haven't said why is this JsonFileSessionStore useful at all? Why did you include it? What benefit does it bring to you?

I mean, it's always better to have a human-readable JSON file than an unreadable DAT one containing bytes.

You can see what the session file contains, what it is all about.

Also, it prevents the file from getting corrupted. Happened to me a few times in the past using FileSessionStore, had to delete the session file and reconnect/auth.

And it's just way better in general for storing data, in my opinion. For example, I currently store a lot of sessions in a SQL database, and doing it with bytes would have been a pain.

I believe a JSON session file is just better in general.

knocte · 2020-09-14T07:20:11Z

I agree, thanks for the explanation. I'll be reviewing this PR today.

knocte · 2020-09-14T08:22:41Z

2 questions:

Why is the commit Set C# language version to 8.0 for all projects needed?
Along with the changes of commit Use JsonFileSessionStore as default TelegramClient ISessionStore, can you provide a migration path so that, if a .dat file is found, it is converted to json first, and then used?

Laiteux · 2020-09-14T08:27:26Z

I wanted to use the ??= operator, which was only available from C# 8.0. I then thought upgrading the LangVersion for all projects wouldn't be a bad thing.
Can you please provide more explanations? What exactly do you want?

knocte · 2020-09-14T08:33:23Z

I wanted to use the ??= operator

Ah, do you actually mind to not use this operator please? I find it quite unreadable.

Can you please provide more explanations? What exactly do you want?

Imagine some software developer uses TgSharp current master branch, and in the future he upgrades to a version that has this feature merged. If we merge this PR as is, what I guess will happen is that his program won't try to open the .dat file, instead it will try to open a saved JSON store file, it will not find it, and create a new one. However, what should happen is that it should be backwards compatible: TgSharp should see the existence of the .dat file (and no existence of JSON store file), then it should convert the .dat file to JSON, and afterwards, load the session from that JSON file.

Laiteux · 2020-09-14T08:37:54Z

Ah, do you actually mind to not use this operator please? I find it quite unreadable.

Ok, sure. Should I then revert 306dc8f8b76c8490e5f92eaf6eb288760d073bf4 too?

Imagine some software developer uses TgSharp current master branch, and in the future he upgrades to a version that has this feature merged...

Got it, will write that tomorrow.

knocte · 2020-09-14T08:39:57Z

Should I then revert 306dc8f too?

Yup because it's not needed anymore. We will upgrade C# when we really need it.

Laiteux · 2020-09-14T10:17:46Z

Tested & working 👌

knocte · 2020-09-14T10:38:51Z

Cool thanks! BTW, are you good at rebasing, separating commits et al?

Laiteux · 2020-09-14T10:39:40Z

Never done that. Honestly this is one of my first "serious" pull requests, I'm still a bit of a beginner with git.

knocte · 2020-09-14T10:41:58Z

So if I ask you to separate your last commit in 2 commits (one that removes the ??= operator, and one that introduces the migration), will you know how to do it?

Laiteux · 2020-09-14T10:45:32Z

I have no idea if I did it the way you wanted, but here we go

knocte · 2020-09-14T10:59:17Z

There's a merge commit, so no :( you should have used interactive rebase

Laiteux · 2020-09-14T11:04:40Z

I have no idea of how to do that, sorry

knocte · 2020-09-14T11:43:45Z

If you give me access to your fork I'll remove the merge commit

Laiteux · 2020-09-14T11:45:42Z

https://github.com/Laiteux/TgSharp/invitations

knocte · 2020-09-14T12:03:41Z

@Laiteux see how I reordered commits, squashed some of them and adjusted commit messages (thanks to the magic of git rebase -i)

Laiteux · 2020-09-14T12:04:53Z

Looks cool!

knocte · 2020-09-14T12:16:01Z

Thanks. By the way I've just realised there may be a couple more improvements to make, I'm going to point them out now, but be careful, given that I pushed with --force into your master branch, I'd recommend to work on a new git clone next.

knocte · 2020-09-14T12:16:34Z

(Otherwise, you may get conflicts, or git merge commits, if you pull.)

src/TgSharp.Core/DataCenter.cs

src/TgSharp.Core/MTProto/Crypto/AuthKey.cs

Laiteux · 2020-09-17T08:51:16Z

However, that would mean it STILL would be included in the binary session file, and others. JsonIgnoreAttribute will only affect the JsonSerializer/JsonConvert. Let me know what you think

knocte · 2020-09-17T08:53:01Z

We don't want the binary session file to store different things than the JSON session file.

Laiteux · 2020-09-17T08:53:33Z

So what should we do? 😕

Laiteux · 2020-09-17T08:56:19Z

Consumer could still get TLUser using MakeAuthAsync and store it himself if really needed.

knocte · 2020-09-17T08:58:20Z

User could still get TLUser using MakeAuthAsync and store it himself.

So, you meant to answer "no" to Anders' below question?:

Isn't that the only way for knowing the current connected/authed user id?

Laiteux · 2020-09-17T08:58:59Z

Yes, this is what I meant.

AndersMad · 2020-09-17T09:01:51Z

Ye, I just added the id from TLUser to the Session persisting (1 line in session and 1 line in client on auth) - the other (TLUser) values are flux anyways - but the user id is nice to know.

Laiteux · 2020-09-17T09:02:53Z

Sorry, what do you mean by that? I don't understand

Ye, I just added the id from TLUser to the Session persisting (1 line in session and 1 line in client on auth)

Laiteux · 2020-09-17T09:04:18Z

Nevermind, got it. Would you mind contributing to this PR?

AndersMad · 2020-09-17T09:04:38Z

@Laiteux In my code to make it work.. I just added UserId prop to the session so it gets persisted and then set it where the TLUser was set in the Cient.

AndersMad · 2020-09-17T09:07:42Z

@Laiteux Yes, but I'm so so bad at git/github - I always f'up something :P .. But was just this to TelegramClient.cs + added the prop to Session

private void OnUserAuthenticated(TLUser TLUser) {
	Session.UserId = TLUser.Id;

Laiteux · 2020-09-17T09:08:55Z

That's fine, I'll add it then rebase. Ok for you @knocte?

knocte · 2020-09-17T09:10:40Z

yes sounds fine; note you may have problems injecting that change in the 2nd commit because the 3rd commit changes stuff around that too (if this is the case, you can just merge the 2nd and 3rd commits into 1, copy+pasting the text from the commit message from the 3rd)

aarani · 2020-09-17T09:50:37Z

No problem if you guys want to remove TLUser but you have to have a GetMe function to get the TLUser object ( if user == null (can be overruled with a force_update arg) )

knocte · 2020-10-12T17:47:28Z

( if user == null (can be overruled with a force_update arg) )

what do you mean with the above @aarani ?

aarani · 2020-10-12T19:50:21Z

( if user == null (can be overruled with a force_update arg) )

what do you mean with the above @aarani ?

@knocte

class Session
{
   internal TLUser TLUser;
}

class TelegramClient
{
   async Task<TLUser> GetMe(bool refresh = false)
   {
      if (refresh || session.TLUser == null)
      {
         session.TLUser = await this.GetUser(self); // pseudo-code
      }
      return session.TLUser;
   }
}

knocte · 2020-10-12T23:44:55Z

I rebased this PR in this branch: https://github.com/nblockchain/TgSharp/commits/wip/jsonSessionStore , can you review @aarani ? (Note: CI might be broken in the last commit, I just have to replace the .dat file with a new .json file, I'll do that tomorrow.)

aarani · 2020-10-13T05:46:14Z

I rebased this PR in this branch: https://github.com/nblockchain/TgSharp/commits/wip/jsonSessionStore , can you review @aarani ? (Note: CI might be broken in the last commit, I just have to replace the .dat file with a new .json file, I'll do that tomorrow.)

LGTM but it probably needs more testing

This will help implementing session storage in JSON format. WIP PRs: - #18 - #29

knocte · 2020-10-13T17:49:15Z

Migrated to System.Text.Json and fixed binary serialization nits pointed by @aarani.
Hey @Laiteux by any chance can you test this before I merge?

It's always better to have a human-readable JSON file than an unreadable DAT one containing bytes. You can see what the session file contains, what it is all about. Also, it prevents the file from getting corrupted. Happened to me a few times in the past using FileSessionStore, had to delete the session file and reconnect/auth. And it's just way better in general for storing data. For example, I currently store a lot of sessions in a SQL database, and doing it with bytes would have been a pain. JSON session file is just better in general. NOTE: Reason for migrating to .NET4.6.2 is to be able to reference .NETStandard2.0 (needed for System.Text.Json). Co-authored-by: Andres G. Aragoneses <[email protected]>

omerjacobi · 2021-04-01T07:06:38Z

what is still missing to finish this pull request?

Laiteux · 2021-04-01T07:09:15Z

Nothing I believe, iirc it works well but we wanted to do some more tests before merging, which we never ended up doing

omerjacobi · 2021-04-01T07:12:09Z

Nothing I believe, iirc it works well but we wanted to do some more tests before merging, which we never ended up doing

Is there anything I can help with? I really like to use this feature in the official NuGet

Laiteux · 2021-04-01T07:13:30Z

Please test the feature on your side and let us know if everything works fine. If so, @knocte will most likely merge it

knocte · 2021-04-01T07:14:16Z

Is there anything I can help with?

@omerjacobi you wanna contribute? tell me your username in telegram and I'll add you to a group where we can chat

omerjacobi · 2021-04-01T07:21:02Z

Is there anything I can help with?

@omerjacobi you wanna contribute? tell me your username in telegram and I'll add you to a group where we can chat

@knocte I send it to your gmail

knocte force-pushed the master branch from c968b2e to 3ea8974 Compare September 14, 2020 06:48

knocte reviewed Sep 14, 2020

View reviewed changes

src/TgSharp.Core/DataCenter.cs Outdated Show resolved Hide resolved

knocte reviewed Sep 14, 2020

View reviewed changes

src/TgSharp.Core/MTProto/Crypto/AuthKey.cs Outdated Show resolved Hide resolved

knocte added a commit that referenced this pull request Oct 13, 2020

Core: refactor around Session vs SessionStore

ef89274

This will help implementing session storage in JSON format. WIP PRs: - #18 - #29

knocte force-pushed the master branch from f605f1d to ef499c6 Compare October 13, 2020 13:19

Trigger build

63052c3

Laiteux closed this by deleting the head repository Sep 13, 2022

Add JsonFileSessionStore #18

Add JsonFileSessionStore #18

Conversation

Laiteux commented Sep 14, 2020

Laiteux commented Sep 14, 2020

Laiteux commented Sep 14, 2020 • edited Loading

knocte commented Sep 14, 2020

Laiteux commented Sep 14, 2020 • edited Loading

knocte commented Sep 14, 2020

Laiteux commented Sep 14, 2020 • edited Loading

knocte commented Sep 14, 2020

Laiteux commented Sep 14, 2020 • edited Loading

knocte commented Sep 14, 2020

knocte commented Sep 14, 2020

Laiteux commented Sep 14, 2020

knocte commented Sep 14, 2020 • edited Loading

Laiteux commented Sep 14, 2020

knocte commented Sep 14, 2020

Laiteux commented Sep 14, 2020

knocte commented Sep 14, 2020

Laiteux commented Sep 14, 2020

knocte commented Sep 14, 2020

Laiteux commented Sep 14, 2020

knocte commented Sep 14, 2020

Laiteux commented Sep 14, 2020

knocte commented Sep 14, 2020

Laiteux commented Sep 14, 2020

knocte commented Sep 14, 2020

Laiteux commented Sep 14, 2020

knocte commented Sep 14, 2020

knocte commented Sep 14, 2020

Laiteux commented Sep 17, 2020 • edited Loading

knocte commented Sep 17, 2020

Laiteux commented Sep 17, 2020

Laiteux commented Sep 17, 2020 • edited Loading

knocte commented Sep 17, 2020

Laiteux commented Sep 17, 2020

AndersMad commented Sep 17, 2020

Laiteux commented Sep 17, 2020

Laiteux commented Sep 17, 2020

AndersMad commented Sep 17, 2020

AndersMad commented Sep 17, 2020 • edited Loading

Laiteux commented Sep 17, 2020

knocte commented Sep 17, 2020

aarani commented Sep 17, 2020

knocte commented Oct 12, 2020

aarani commented Oct 12, 2020 • edited Loading

knocte commented Oct 12, 2020

aarani commented Oct 13, 2020

knocte commented Oct 13, 2020

omerjacobi commented Apr 1, 2021

Laiteux commented Apr 1, 2021 • edited Loading

omerjacobi commented Apr 1, 2021 • edited Loading

Laiteux commented Apr 1, 2021

knocte commented Apr 1, 2021

omerjacobi commented Apr 1, 2021 • edited Loading

Laiteux commented Sep 14, 2020 •

edited

Loading

Laiteux commented Sep 14, 2020 •

edited

Loading

Laiteux commented Sep 14, 2020 •

edited

Loading

Laiteux commented Sep 14, 2020 •

edited

Loading

knocte commented Sep 14, 2020 •

edited

Loading

Laiteux commented Sep 17, 2020 •

edited

Loading

Laiteux commented Sep 17, 2020 •

edited

Loading

AndersMad commented Sep 17, 2020 •

edited

Loading

aarani commented Oct 12, 2020 •

edited

Loading

Laiteux commented Apr 1, 2021 •

edited

Loading

omerjacobi commented Apr 1, 2021 •

edited

Loading

omerjacobi commented Apr 1, 2021 •

edited

Loading