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

Add bindings for pg_query_deparse_protobuf #32

Merged
merged 2 commits into from
May 2, 2024

Conversation

tomquist
Copy link

This exposes deparse functionality to node.js. As of this issue the recommended approach is to generate a protobuf message from the JSON parse tree. One blocking issue is that protobufjs doesn't honor the json_name attribute so it isn't able to parse the existing structure into a protobuf message. There's a PR to add this, however, this seems to be stale atm. In this PR I worked around this by directly pointing to the fork and generating a static JS model from it that I checked in. By checking in the model we could also point back to the original library, however, this makes updating the static model more cumbersome.

This pull request adds bindings for pg_query_deparse_protobuf to expose deparse functionality for Node.js. Currently, the recommended approach is to generate a protobuf message from the JSON parse tree, but the protobufjs library doesn't honor the json_name attribute, which blocks parsing of the existing structure into a protobuf message. Although a PR to address this issue is available here, it seems stale at the moment.

In this PR, a workaround has been implemented by directly pointing to the fork and generating a static JS model from it that is checked in. While checking in the model allows for pointing back to the original library, it also makes updating the static model more cumbersome.

@tomquist tomquist mentioned this pull request Mar 26, 2023
@jgoux
Copy link

jgoux commented Mar 30, 2023

This is awesome, thanks for your contribution! 🥳

@benasher44
Copy link
Contributor

@pyramation pyramation mentioned this pull request Feb 20, 2024
@pyramation pyramation deleted the branch launchql:tom/deparse-pr February 21, 2024 03:08
@pyramation pyramation closed this Feb 21, 2024
@pyramation
Copy link
Collaborator

oops, I deleted master and a few others in an attempt to have more clarity around branches, keep similar naming convention as libqg-query and it automatically closed this PR — re-opening :)

@pyramation pyramation reopened this Feb 21, 2024
@pyramation pyramation changed the base branch from master to pg_query_deparse_protobuf February 21, 2024 03:53
@pyramation pyramation changed the base branch from pg_query_deparse_protobuf to tom/deparse-pr May 2, 2024 07:04
@pyramation
Copy link
Collaborator

Would love to breathe life back into this! I've actually written protobuf parsers from scratch (see https://github.com/cosmology-tech/telescope)

@tomquist was this PR working for you? If so, I can look into the details of the option json_name and handle this

I could also make a nice pipeline for updating the proto.js that gets generated. I'll take a deeper look :)

@pyramation
Copy link
Collaborator

$ npm run test

> [email protected] test
> mocha --timeout 5000



  Queries
    Sync Parsing
      ✓ should return a single-item parse result for common queries
      ✓ should support parsing multiple queries
      ✓ should not parse a bogus query
    Async parsing
      ✓ should return a promise resolving to same result
      ✓ should reject on bogus queries
    Deparsing
      ✓ async function should return a promise resolving to same SQL
      ✓ sync function should return a same SQL
      ✓ should reject on bogus input
    Fingerprint
      sync
        ✓ should not fingerprint a bogus query
        ✓ should fingerprint a query
      async
        ✓ should not fingerprint a bogus query
        ✓ should fingerprint a query

  PlPgSQL (async)
    ✓ should parse a function


  13 passing (12ms)

NOTE: yarn doesn't work with Tom's branch, but npm install does :)

I think this is a great start, I can work with this!

@pyramation pyramation marked this pull request as ready for review May 2, 2024 20:49
@pyramation
Copy link
Collaborator

I'm gonna merge this PR into a dev branch, and work on this!

@pyramation pyramation merged commit 9b6a671 into launchql:tom/deparse-pr May 2, 2024
@pyramation pyramation mentioned this pull request May 2, 2024
@pyramation
Copy link
Collaborator

new PR #63

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