SQLite support

[tab00]

After adding extends SQLDataSource to the datasource class I get the above error and the component doesn't render.

It happens even if I don't call any of the functions (getBatched(), getCached(), getBatchedAndCached())

If I remove just those two words (extends SQLDataSource) then the app works fine.

Is this a bug?

[cvburgess]

can you post a more complete snippet? I have never encountered this issue.

[tab00]

import knex from "knex"
import { SQLDataSource } from "datasource-sql"

const knex1 = knex({
  client: "sqlite3",
  connection: { filename: "./sqlite.db" },
  useNullAsDefault: true
})

const MINUTE = 60 * 1000

export class DSBooks extends SQLDataSource {
  constructor() {
    super()
    this.knex = knex1
  }

  async getBooks({ idOfOwner }) {
    console.log(`In data source function getBooks. idOfOwner: ${JSON.stringify(idOfOwner)}`)

    const query = knex1.select().from("books")
      // return await this.getBatchedAndCached(query, MINUTE)
      .then((books) => {
        console.log(`books: ${JSON.stringify(books)}`)
        return books
      })
    return await query
  }

  async bookAdd({ book }) {
    console.log(`In data source function bookAdd. book: ${JSON.stringify(book)}`)

    return await knex1("books").insert(book)
      .then(idArray => Object.assign(book, { id: idArray[0] }))
  }
}

[cvburgess]

that looks fine, but how are you calling and using the DataSource?

[cvburgess]

FYI: typically that error is not stemming from a DataSource, it is caused by a GraphQL type referencing itself in your schema. See apollographql/apollo-server#1557

[tab00]

This is my schema:

export const typeDefs = `
type Book {
  id: String
  title: String
  author: String
  idOfOwner: String
}

type Query {
  books: [Book]
}

type Mutation {
  bookAdd(title: String!, author: String!): Book
}

type Subscription {
  latestBook(idOfOwner: String!): Book
}
`

There is no type referencing itself .

[tab00]

that looks fine, but how are you calling and using the DataSource?

The error occurs even if the DataSource is not used.

[tab00]

Here is the DataSource class being passed into ApolloServer instantiation:

  const server = new ApolloServer({
    typeDefs,
    resolvers,
    dataSources: () => ({
      dsBooks: new DSBooks()
    }),
    context: async ({ req, connection }) => {
      if (connection) { // check connection for metadata
        const token = req.headers.authorization || ""
        return { token }
      }

      return { user: await getUser(req.headers.authorization) }
    },
    uploads: false,
  })

[tab00]

resolvers:

export const resolvers = {
  Query: {
    books: async (root, args, context) => {
      console.log(`Query context: ${JSON.stringify(context)}`)
      // if (!context.user) throw new Error("Please log in.")

      const books = await context.dataSources.dsBooks.getBooks(context.user ? { idOfOwner: context.user._id } : {}) //Meteor user available because of https://github.com/apollographql/meteor-integration
      return books
    }
  },

  Mutation: {
    bookAdd: async (root, { title, author }, { dataSources, user }) => { //{ title, author } is args, { dataSources, user } is context. Called "destructuring assignment"
      console.log(`In bookAdd mutation. user: ${JSON.stringify(user)}`)

      if (user === undefined) throw new Error("Please log in.")

      const latestBook = await dataSources.dsBooks.bookAdd({ book: { title, author, idOfOwner: user._id } })
      pubSub.publish(subChannelBookAdded, { latestBook })
      return latestBook
    }
  },

  Subscription: {
    latestBook: {
      subscribe: withFilter(() => pubSub.asyncIterator(subChannelBookAdded),
        (payload, variables, context) => {
          return payload.latestBook.idOfOwner === variables.idOfOwner && payload.latestBook.idOfOwner === context.user._id //only update books added by current logged in user. return true (or don't use withFilter()) if you want everyone to see. user is added to context via SubscriptionServer onConnect.
        }
      ),
    }
  },
}

[tab00]

I've added getBooks() and bookAdd() functions to DSBooks class in the code in #9 (comment)

[cvburgess]

can you verify that it is in fact the SQL Data Source that is causing the issue?

you are currently not even using the methods from the class so im not sure what would be causing this error. Try replacing this with a placeholder class and see if the issue persists

[tab00]

I wrote a simple SQLDataSource class

import { DataSource } from "apollo-datasource"
class SQLDataSource extends DataSource {
  initialize(config) {
    this.context = config.context
    this.db = this.knex
  }
}

export class DSBooks extends SQLDataSource {

I have found that this.context = config.context causes the error, as when I remove that the app runs fine.

[tab00]

If I replace this.context = config.context with this.context = Object.assign({}, config.context) (and add all the other SQLDataSource and SQLCache code) then the app runs.

Only getCached() works. Is this the right way to use getBatched()?:

  async getBooks({ idOfOwner }) {
    const query = knex1.select().from("books")

    return this.getBatched(query)
  }

[cvburgess]

what version of apollo server are you using?

[tab00]

Node package dependencies:

"dependencies": {
    "@babel/runtime": "^7.2.0",
    "apollo-cache-inmemory": "^1.3.12",
    "apollo-client": "^2.4.8",
    "apollo-link": "^1.2.6",
    "apollo-link-error": "^1.1.5",
    "apollo-link-http": "^1.5.9",
    "apollo-link-ws": "^1.0.12",
    "apollo-server-express": "^2.3.1",
    "datasource-sql": "^0.1.4",
    "graphql": "^14.1.1",
    "knex": "^0.16.3",
    "meteor-node-stubs": "^0.4.1",
    "node-fetch": "^2.3.0",
    "prop-types": "^15.6.2",
    "react": "^16.7.0",
    "react-apollo": "^2.3.3",
    "react-dom": "^16.7.0",
    "sequelize": "^4.42.0",
    "sqlite3": "^4.0.6",
    "subscriptions-transport-ws": "^0.9.15"
  },

[tab00]

If I replace:

SQLDataSource/SQLCache.js

Line 14 in 221011c

return this.loader.load(queryString).then(result => result && result.rows);

with

    return await this.loader.load(queryString)

Then getBatched() works as result.rows is undefined.

[tab00]

Similarly, if I comment out

SQLDataSource/SQLCache.js

Line 38 in 221011c

.then(result => result && result.rows)

then getBatchedAndCached() works.

[cvburgess]

hmmm i don't think you're "fixing" it as much as you're masking the issue.

Could you create a codesandbox? I am struggling to reproduce your issues.

[tab00]

I made a new GitHub repository for you: https://github.com/tab00/test-datasource-sql

The problem is at:
https://github.com/tab00/test-datasource-sql/blob/master/imports/api/server/books/DSBooks.js#L10

[tab00]

The problem with result.rows is probably a separate issue. the resolved result from load() is an array, and rows is not a property of an array. Why && result.rows anyway, instead of just returning the result?

[cvburgess]

@tab00 if you are familiar with knex, I use their .raw() under the hood to execute the batching. .raw() results in a different object than a normal knex request. try it for yourself locally :)

Thank you for the repo, will pull and look.

[cvburgess]

That repo has a lot going on... III'm able to get a much simpler example working with no issues.

I don't think the issue is with this repo, but rather the configuration of your app.

Sorry, I am unable to help further at this time.

[tab00]

It's a simple meteor app. Did you try to run it?

Can I see that much simpler example?

[cvburgess]

I am able to reproduce by switching to SQLite - it appears the .raw() response is different than it is for postgresql for whatever reason

[cvburgess]

Here is some code with postgres vs sqlite https://github.com/cvburgess/9-sandbox

( you willl need to set up your database locally for pg )

[cvburgess]

postgres works, sqlite does not.

If youd like to open a PR to add sqlite support or document its current broken behavior, please do!

[tab00]

I am able to reproduce by switching to SQLite - it appears the .raw() response is different than it is for postgresql for whatever reason

I assume you are referring to the problem with result.rows and not the original error "Converting circular structure to JSON" which occurs on import of datasource-sql before any query is executed.

I think the common standard should be that execution of a query, regardless of DB, returns an array of JavaScript objects (and an empty array if there are no matches). SQLite already does that, so if postgresql does not return an array then it should be postgresql related code that should be changed. I don't plan on using postgresql.

I think you can add the condition check knex.client === "pg" and transform the result into an array.

[tab00]

Here is some code with postgres vs sqlite https://github.com/cvburgess/9-sandbox

I changed that code to use SQLite instead (as I don't use postgresql), and I don't get the original error "Converting circular structure to JSON", so that problem may be related to Meteor and its config.context.

Replacing this.context = config.context with this.context = Object.assign({}, config.context) in SQLDataSource allows the app to run, but I'm not sure if cloning instead of pointing to the original object instance would cause other problems.

[cvburgess]

I'm doing exactly what RESTDataSource is doing so I am not tempted to make changes there.

The meteor part is a little strange, I do not personally do meteor development so it's not something i can help with much.

[tab00]

I've solved the "Converting circular structure to JSON" problem - it was caused simply by trying to print context to console in the resolver. Sorry about that.

So now the only problem is that postgresql doesn't return an array from a Knex raw query. There is no problem when using an SQLite DB.

I've also simplified the code in my GitHub repository by removing React and refactoring code: https://github.com/tab00/test-datasource-sql

[cvburgess]

Correct, hence why i reanamed this issue.

Knex seems to return different results for raw queries to these different dbs.

Please open a PR if you have a solution :)

[tab00]

I would open a PR if the problem was with SQLite. But the problem is with postgresql which I don't use. So you'll need to write the code to transform whatever postgresql returns to an array of JavaScript objects.

[cvburgess]

if the problem was with SQLite

I would not say there is a "problem" with either, the implementations need to be made similar and consistent. I personally use PostgresSQL and have zero issues so we may have different expectations?

I do not control Knex so i cannot change what is returned from that library. All I can do (or anyone willing to contribute) is conditionally return the appropriate results so that an array is always returned from the SQLCache functions. That is already happening for PG (hence the .rows) but that same logic is not valid for SQLite. We simply need to account for the differences in Knex. MySQL and MSSQL may also return slightly different responses that could cause errorrs.

[tab00]

I personally use PostgresSQL and have zero issues so we may have different expectations?

Not everyone uses PostgresSQL but would expect this package to handle other SQL databases.

that same logic is not valid for SQLite.

For SQLite it's even simpler - the raw query result is already an array of JavaScript objects. It is the postgresql-specific code that you had written (.rows) that is causing an error, because a plain array doesn't have a rows property.

You could add this.knex = knex in the SQLCache constructor so that you can access the client property and use a switch to determine whether transformation is needed before returning. e.g.:

  async getBatched(query) {
    const queryString = query.toString();

    const result = await this.loader.load(queryString)

    switch (this.knex.client) { //an array of JavaScript objects must be returned
        case "pg" :
            return result && result.rows //assuming  `&& result.rows` transforms `result` into an array, but only postgresql users would know
            break
        default:
            return result
    }
}

[cvburgess]

Haha, im not disagreeing with you in the slightest, I am simply stating why that feature does not currently exist. Its not something I have tested and built. Please feel free to build, test, and open a PR to support your workflow! It may take me quite a while to get to this on my own as I am busy with many projects and a full-time job.

[tab00]

I made a pull request as requested.

I may have come across another issue: it seems that cache entry doesn't expire.

[cvburgess]

Correct - there is no default value for the cache.

Its not a bug, it configurable. You can set any amount of expiry you would like, or none at all.

We can improve documentation around this for sure.

[tab00]

What I mean is that even if I set ttl to 1 cache.get(cacheKey) still returns the cache entry, when it should not as that entry should have expired already.

[cvburgess]

What are you using as your cache?

[tab00]

InMemoryLRUCache as instantiated in the SQLCache constructor:

constructor(cache = new InMemoryLRUCache(), knex) {

[cvburgess]

Now supported in v0.1.7