-
-
Notifications
You must be signed in to change notification settings - Fork 180
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
Full business logic in JavaScript #279
Comments
To expand the idea a little bit, if we could implement this, something like this could be possible too: // mask the email in results if authenticated user id is not 1 (admin)
function request(vars, request) {
let result = graphql(request.query, request.bindings);
// `request.query` is the incoming GraphQL query with $foo params intact
// `request.bindings` is any of $foo substitutions, so we can send the request to graphql ourselves
// request.userId contains the id from auth module
if(request.userId !== 1) {
// ok, maybe a better way would have been to graphql() the user
// and see if user is is_admin or something, but just for brevity
result.email = 'sorry, you cannot see this, only admins can';
}
return result;
} which would be the equivalent of function request(vars) {
...
}
function response(json) {
// if userid != 1 -- not implemented and cannot check the databsae
json.email = 'sorry, you cannot see this, only admins can';
} but we don't really need |
Backwards compatibility issue: if this is implemented, it changes Sample query: query GetUsers(user_name: $name, first: 30) {
user {
id
}
}
{name: "Jim"} function process(req) {
// req.type
// 'query' or 'mutation'
// req.vars(?) has query/mutation params
// {user_name: "Jim", first: 30}
// req.query has the text of query with $xyz intact
// `query GetUsers(name: $name) {\n user....'
// req.bindings has {xyz: 123} data
// {name: "Jim"}
// then we could easily do:
let result = graphql(req.query, req.bindings);
// req.schema has something like an object that has incoming graphql query parsed
// so that we can figure out which fields the user expects without manually parsing the req.query string
// ? maybe {"user": ["id"]}, not really sure
// req.userId = user ID from the auth module
// 2 or null
// req.headers - http headers of the request
// {'authorization': 'Bearer 12345', 'user-agent': 'Netscape Navigator 5.0'}
// req.remoteIp - IP of the remote server, this would allow some custom IP-based rate limits, like per hour or so
// '8.8.8.8'
return result
} But in this case we definitely need to change the function name from |
A few more advanced thoughts:
Now that would have been really powerful, since we could call some other services too (via But that's something more advanced, though generally modern JS devs would assume you could both do a synchronous function (just like now) and |
This also opens up possibilities to do some advanced validations - like whether this user could really |
Caveat: during the development GraphJin should not add queries from Also, possibility to do transactions from JS could be really cool (so that graphql calls are in a transaction). |
I really like this and would be open to exploring this further. When I added the @script tag I did think of taking it further with custom resolvers. I added some more relevant details to your other issue on the topic #280 As for the |
I don't think it's possible, since you need to set it to both true and false at the same time :) Here's what I mean. Let's say, that I have some regular query getCustomUsers {
email
} getCustomUsers.js: function process(req) {
const oldUsers = graphql(`query getOldUsers oldUsers(limit: 5) { email }`);
const newUsers = graphql(`query getNewUsers newUsers(limit: 5) { email }`);
return oldUsers + newUsers;
}
So, I want allow list to be enabled, but getOldUsers and getNewUsers will not be in the allow list, since I don't want users to run them. But unless we ignore the allow list for internal JS calls, these graphql calls will be blocked on production and getCustomUsers will fail. |
Ok I kinda understand what you're getting at. Can you try the same queries without the name (getOldUsers, getNewUsers) queries without a name are not saved to the allowlist (Not sure if they will work as you want but I'd like to find out). I do see why queries from the JS script should not be accessible publicly outside of the JS script. I'll look into this and see if I can have a fix. Also unrelated but you can combine the above queries. function process(req) { |
The example is superfluous, of course. Imagine there's some "if" after the first query that checks some condition, then it makes a second query.
The problem as I see it, is that this approach requires huge discipline. If anyone accidentally copies a named query from GraphQL explorer into an internal call - that instantly becomes whitelisted. Imagine that call is something like "increase the balance" (after checking that the user is entitled to that) or "unban a bad user" (after some ip-based rate limits checks in Redis). EDIT: Also, I just realized that even if we used non-named queries - that means that as soon as we go to production, they will not work, so all internal functions will go down. |
It seems that this isn't even that hard to do:
So, basically, seed.js already implements at least one of the requirements perfectly! |
FYI This has been implemented. https://www.reddit.com/r/golang/comments/nekgou/graphjin_graphql_to_sql_compiler_with_javascript/ |
@dosco I'm aware of |
|
Sorry it's a long post I think I missed the point let me re-read. |
I think the 3 points you made are valid and probably not very hard to implement. Let me look into them. Yes, I agree it would do away with the need for response but probably doesn't hurt to just leave it in.
I need to think through 3 a bit more what would be the point of the selector if you are only using the graphql query to execute a script not sure if this is even valid graphql?
Also I did want to implement |
Well, the point would be that in absense of a way to do it in GraphJin, we need to do this in some other backend thing (node.js/Python/Ruby/PHP, etc...) and if we have that backend, then we have no need in GraphJin, because otherwise we'll have a wild mix of node/graphjin that would be very hard to test. That's why we don't use GraphJin now - it doesn't allow us to move all the logic to scripts and use it as a backend and since we can't do it, then we need another backend and if we have another backend - it doesn't makes sense to have 2 backends and therefore GraphJin is out. |
Fair enough we could probably add the ability to run scripts from our actions (we allow you to create http endpoints called actions in GraphJin) right now they can only run SQL queries but we can extend those to run scripts. As for running two backends I know several users who have two backends once for custom scripts others for crud queries via graphjin it still saves them months of time that they would spend writing all those apis instead of just the special ones. |
I'm not saying that GraphJin isn't useful, it is. But for the sites that we manage, we need to do a lot more than CRUD (like autorization - checking if user is banned, has permissions, etc... checking rate-limits, logging). If we do it in node.js or smth like that, it means that we only replace SQL calls with GraphJin calls in our node.js code - that's not much of time saving here, since both SQL and GraphQL are about the same size (unless you use deep joins, which your shouldn't in big databases), but at the same time we have to deploy and monitor one more thing (GraphJin). |
graphjin now works as a nodejs app so this usecase can be implemented in javascript |
@dosco Now, that is awesome! Kudos! |
Using joins for even large databases is fine most of these are web queries so already limited to 5 or 10 results and then we use cross lateral join to pull in other related data (eg. blog post + author + like authors etc) else you'd have to munge this data together in the app code. additionally you also get the ability to do recursive queries (eg. comment + reply + reply_to_reply threads), built in efficient pagination using a cursor, remote joins with (eg. pull in stripe payment info for a user) At scale as your apis grow in number it really saves a ton of time and you always get the most efficient query no sudden n+1 somewhere cause that one dev was not familiar with joins. Also most the words databases are internal company / org ones at non-tech companies where talent is a bigger challenge. A centralized config file allows you to set rules in one place around blocking certains columns for certain roles or entirely, adding certain filters to certain tables and these rules will apply across all your apis. Addiitonally since |
@dosco Is it possible to add custom business logic directly in the GraphJin Standalone Service? |
You can use |
Awesome project, thank you Vikram!
I'm thinking about something like a "forgot password" functionality. I don't think it's quite possible yet to implement it fully in GraphJin yet, but maybe with a few small changes it could be done (and it opens a lot of other possibilities too). (Please correct me if this is all already possible).
Basically, a user requests something like this:
where
forgotPassword.js
could be something like this:There are three things that I think are missing now in the GraphJin, that could make it hugely more useful:
request(...)
I think this opens huge possibilities - you could have a whole additional API without running node.js or something. Full custom business logic right in GraphJin
Then of course you'd need something that validates the code after it was sent, so it could be something like
and
resetPassword.js
:I don't know - maybe instead of
return {ok: false, message: "xyz error"}
it would be better tothrow
an exception, but I think you get the idea.This might actually also help with the problem that YugaByteDB guys told about - where a complex query like
query getHome
from older mobile clients could be intercepted by the backend via a customrequest
function, be processed in a more efficient manner (by runninggraphql()
calls inside) and return the data with the same schema, but using a different way to run it.Thank you!
The text was updated successfully, but these errors were encountered: