Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote Server #695

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

erik-gross-hardt
Copy link

@erik-gross-hardt erik-gross-hardt commented Jul 14, 2023

Remote Server

Hello Frappe-Books Team,

I'm writing to submit a pull request for the remote connection feature I've been working on. I wanted to emphasize that this implementation is very basic (and unconventional) but I have had a great experience using it so far. I have begun working on a remote connection feature, which aims to simplify the way users can share there books by establishing a connection to a remote server. Although the feature is already fully functional, I would like to highlight that security measures have not been incorporated at all yet. Consequently, this pull request is intended for review and discussion.

How it works

I have implemented a custom Knex database client that extends the standard better-sqlite3 client. My client intercepts the communication just before establishing a native SQLite connection, redirecting the SQL queries that would normally hit the SQLite database directly, and instead sending the queries to a server. This server then establishes the necessary communication with the SQLite file and responds with the query results.

Here is a quick illustration:

image

Things I had to change

  • Add another button with a modal to the database selector allowing the user to enter the server adress

  • Add a util function that allows me to test whether the database path is a URL

  • Bypass the interaction with the file system if the database path is a URL in various places

  • Dynamically use the custom client in the database core file when the database path is a URL

  • Enhance the database selector by incorporating an additional button with a modal window, enabling users to input the server address.

  • Add a utility function that verifies whether the database path corresponds to a URL.

  • Alter various sections along the way to bypass file system interactions when dealing with a URL-based database path.

  • Dynamic use of the custom client within the database core file, when the database path is a URL.

The Server

The server implementation can be found here frappe-books-server
I have also build a docker-image

That's it for now, I hope there may be some value to this. As said I had a great experience using it so far, while working with in a team. I am sure there is a better way to implement server connectivity but I doubt that there will be a simpler one.

Thank you,
Erik

@promexio
Copy link

Erik,
Sounds beautiful. Is it already in the current version? How can I enable this?

--Gabriel

@erik-gross-hardt
Copy link
Author

To "enable" this you will have to build the frappe client from source. Just clone my repo and yarn build it. Follow the instructions here to setup a server instance.

@solancer
Copy link

This is brilliant, @erik-gross-hardt Thankyou for working on this

@18alantom
Copy link
Member

@erik-gross-hardt thanks for raising this PR. It's an unconventional but pretty smart way of adding remote server support.

Since this is mostly a proof of concept, here're a couple of details off the top of my head:

  • For proper remote server support, it'd require authentication and isolation of all the SQLite databases on the server. Since we're essentially sending built queries, if the security part isn't handled well you can have SQL injection from one instance affecting other instances.
  • Queries in Frappe Books have been written in a way that assumes that there won't be any network in the middle, so you can have un-batched high frequency queries and queries that pull entire tables to be parsed in the renderer process. These will still work as you'd mentioned, but due to their inefficiency (when accounting for network latency and single servers handling multiple instances) they'll stress your server out.
  • Remove server might cause users to attempt multi-user setups, so the server would have to be smarter to account for say users on different instances (same db) creating invoices at the same time, and pushing the updates to either of the open instances.

Nevertheless, I think the crux of your idea—i.e. using a custom Knex client to send queries— still stands, and will mostly be the way remove server support is incorporated into Frappe Books.

@18alantom 18alantom added the DONT MERGE PR is not to be merged due to incomplete implementation or other issues. label Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DONT MERGE PR is not to be merged due to incomplete implementation or other issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants