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

Columns [meta] and [type] included in mssql query #53

Open
rnam415 opened this issue Apr 20, 2017 · 21 comments
Open

Columns [meta] and [type] included in mssql query #53

rnam415 opened this issue Apr 20, 2017 · 21 comments

Comments

@rnam415
Copy link

rnam415 commented Apr 20, 2017

When connecting to a SQL Server instance, [type] and [meta] columns are ending up in the raw sql query.

"errors": [
{
"status": "500",
"code": "EUNKNOWN",
"title": "An unknown error has occured",
"detail": "Something broke when connecting to the database - Invalid column name 'meta'."
}
]

SELECT [id], [type], [meta], [first_name], [last_name], [phone_number], [middle_name], [title], [suffix], [email_address], [birth_date], [date_created], [date_modified], [address_line1], [address_line2], [address_line3], [address_line4], [city], [state], [postal_code], [country], [clark_region] FROM [workers] AS [workers];

@paparomeo
Copy link
Contributor

We have never tested with MS SQL Server. It looks like meta is a reserved name in SQL Server and we'll need to escape it. I'll try to have a look at this, but meanwhile if you want to submit a PR with a fix it will be welcome.

@mrrusof
Copy link

mrrusof commented Jul 27, 2017

The same happens when you use postgres. Is inclusion of attributes type and meta intentional? If so, what is the purpose for each one?

@mrrusof
Copy link

mrrusof commented Jul 27, 2017

Cool project btw!

@rnam415
Copy link
Author

rnam415 commented Jul 31, 2017

I did not pursue this issue any further

@pmcnr-hx
Copy link
Contributor

MSSQL is now available on Linux so I'll try to reproduce this soon and hopefully add MSSQL as unofficially supported (not sure if it is possible to run it on Travis).

@aledalgrande
Copy link
Contributor

aledalgrande commented Sep 26, 2017

What about postgres? Is it possible to exclude these fields when using that dialect? I don't know why it's trying to pass a value to a type column, which is not needed.

@pmcnr-hx
Copy link
Contributor

I need a better look at the code to refresh my memory. I'll try to get round to it today.

@aledalgrande
Copy link
Contributor

Trying to look at this one too, it is a blocker for me right now.

@championswimmer
Copy link
Contributor

@aledalgrande what exactly is the problem in postgres ?

@aledalgrande
Copy link
Contributor

aledalgrande commented Sep 27, 2017

@championswimmer when I call a resource creation endpoint I see this from postgres:

ERROR:  column "type" of relation "<my_resource_name>" does not exist at character 34
STATEMENT:  INSERT INTO "<my_resource_name>" ("id","type") VALUES ('c3a32f92-bcbf-4f48-8a34-9747417d628c','<my_resource_name>') RETURNING *;

I don't have the type column, so it fails. Why is it trying to save it? The type shouldn't need to be stored right?

(keep in mind I create my own models with sequelize separately)

@championswimmer
Copy link
Contributor

@aledalgrande done

@paparomeo
Copy link
Contributor

I'll have a better look at this during the weekend and see if it fits with the design philosophy of the jsonapi-server handlers. However, the idea for the data store adapters / handlers is that the model is generated by the handler and should be opaque. When you already have your own model generated externally the recommend approach is to write a custom handler.

@fny
Copy link

fny commented Oct 16, 2017

So I'm having this same issue using sqlite, it's querying for meta despite me never specifying it as an attribute. Is there an expectation that the table contains both a type and meta column?

@paparomeo
Copy link
Contributor

paparomeo commented Oct 16, 2017

The short answer is: yes, as explained in my comment above, The structure of the table is automatically generated by the handler and should be opaque to applications. Also, SQLite is not officially supported at the moment. 4 of the automated tests are failing for SQLite and until those pass we cannot support it officially. Contributions to help figure out what the problem is and fixing it are welcome.

@rnam415
Copy link
Author

rnam415 commented Oct 16, 2017

It's been a few months now since I last looked at this but I clearly remember that the meta and type columns are not being added by the handler. Those columns are being added to the query much deeper in the application - if I remember correctly, possibly even in the Tedious module.

@paparomeo
Copy link
Contributor

You're most probably correct. I'll have to have a more careful look at the code. On a brighter note it appears that the latest version of Sequelize fixes the issues I was still seeing with SQLite tests failing (I had already traced it to a problem with Sequelize, so it's good to see it's fixed) and I have a PR almost ready to add SQLite to the list of supported databases since all the tests are now passing. Should be able to merge it on Wednesday. Then I'll dig deeper into this issue.

@rnam415
Copy link
Author

rnam415 commented Oct 17, 2017

Great. I still have my environment available, i can do some testing as well

@fny
Copy link

fny commented Oct 17, 2017

@paparomeo It would be useful to make this clear in the documentation. I have been assuming that these backends were schema agnostic: given some attribute specification in jsonapi-server, any database should work with any matching schema. My hope was I could use this to turn an existing database into a read only API. It wasn’t clear that these two columns were prerequisites.

@a-wf
Copy link

a-wf commented Dec 5, 2018

So... It is still not fixed yet?

I have the issue too, about the "type" and the "meta"

@twss
Copy link

twss commented May 1, 2019

I'll have a better look at this during the weekend and see if it fits with the design philosophy of the jsonapi-server handlers. However, the idea for the data store adapters / handlers is that the model is generated by the handler and should be opaque. When you already have your own model generated externally the recommend approach is to write a custom handler.

type and meta are, in effect, virtual properties of serializing the model, so shouldn't be persisted to the backing store. If you're using mongoDB (or some other schemaless datastore) as the backing store then this will be hiding the bug.

type alone is redundant in the context of a table/collection as it will always be the same value.

@westandy-dcp
Copy link

westandy-dcp commented Jul 30, 2019

I'm working on a fork with an mssql option. In the default modelGenerator, we set type to STRING instead of VIRTUAL since MSSQL does not understand VIRTUALs. And meta we just added meta to the table.

Trying to setup commands to run the tests is insane. I don't have mysql, psql, sqlcmd, etc. And I'm on a Mac. (Face palm)

We were able to get an Express server on Node using JagQL, and a forked MSSql of this library to work on a MSSql VM on Azure.

Here's the fork:
https://github.com/DCPMidstream/jsonapi-store-relationaldb (if you can't see it, I'll make a PR to this repo)

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

No branches or pull requests

10 participants