CSV Import #418

budden · 2019-02-15T18:30:31Z

Hi! I picked up a closed PR #308 for issue #281 and fixed all issues you mentioned. The only problem is that I'm unable to do make text. Make asks password several times and fails. I changed postgres user password, but it didn't help. I guess something is wrong with my setup. Application itself works fine. Still there are not system tests for CSV import, but I'd like to add them only when I'm sure I can run test suite. Can you please elaborate how to set things up?

sosedoff · 2019-02-15T18:42:59Z

Are you on OSX? It might have something to do with database credentials. Here's my pg_hba.conf file:

# TYPE  DATABASE        USER            ADDRESS                 METHOD

# "local" is for Unix domain socket connections only
local   all             all                                     trust
# IPv4 local connections:
host    all             all             127.0.0.1/32            trust
# IPv6 local connections:
host    all             all             ::1/128                 trust
# Allow replication connections from localhost, by a user with the
# replication privilege.
local   replication     all                                     trust
host    replication     all             127.0.0.1/32            trust
host    replication     all             ::1/128                 trust

Make sure you have postgres user created. Other than that you should be able to run make test without any problems.

sosedoff

Posted a review. I will also try to run this branch locally some time this weekend.

sosedoff · 2019-02-15T19:57:39Z

pkg/api/api.go

+	req.ParseMultipartForm(0)
+	table := req.FormValue("table")
+
+	field_delimiter, err := ParseFieldDelimiter(req.FormValue("field_delimiter"))


Use camelCase for vars: fieldDelimiter

sosedoff · 2019-02-15T19:59:35Z

pkg/api/api.go

+	field_delimiter, err := ParseFieldDelimiter(req.FormValue("field_delimiter"))
+	if err != nil {
+		log.Print(err)
+		c.JSON(400, Error{err.Error()})


Use badRequest(c, err) instead of c.JSON(400, Error{err.Error()}). Same applies to the rest of the error checking.

sosedoff · 2019-02-15T20:01:14Z

pkg/api/api.go

+	createQuery := statements.CreateNewTableQuery(table, header)
+	insertQuery := statements.GenerateBulkInsertQuery(table, header, len(data))
+
+	_, err = db.NewTable(createQuery)


You can add a db.CreateTable / db.DropTable functions instead of using db.NewTable which does not do any specific work, you can use db.query instead.

Sorry, I didn't understand this.

Yep, i didn't realize at which package i was looking at a the time. db.query is not available in this case.
I suggest you rename NewTable func into something else, like QueryWithoutHistory, QueryInternal, or Exec so that we can use it for any queries that don't need to be recorded.

sosedoff · 2019-02-15T20:03:13Z

pkg/api/api.go

+		return
+	}
+
+	result, err := db.BulkInsert(insertQuery, statements.Flatten(data))


We should cleanup that newly created table (and only if it was created during import) if the insert fails. Alternatively you can perform the import in a single transaction so if something fails the whole import is rolled back and there would not be any unwanted artifacts.

Alternatively you can perform the import in a single transaction

There is a contradiction. Huge transactions are bad. So if a table is really huge, it is impractical to populate the entire table in a single transaction. But if we use multiple transactions, then to clear the table we must issue an additional "truncate table" or "drop table" statement explicitly. Then, what if the import failed due to lost sql server connection? In this case we can't enforce cleanup. So the state of the table after failure depends on the nature of failure.

So if I was to decide, I would not do that. If something failed, I would leave cleaning table up to the user. I would just document that and that's it. So, failures get simpler to understand: whenever failure happened, user knows that (s)he must clean the table by himself.

Im fine with your proposal, that approach would be similar to the sql import (using psql) and in case of an error it's up to the user to decide what to do next.

budden · 2019-02-16T07:40:50Z

Are you on OSX?

I'm on Debian Stretch, but I did some manual setup when installing postgres (forgot them already). I think first of all I'll reinstall postgress afresh in an official way, and then retry.

budden · 2019-02-16T12:34:28Z

Here's my pg_hba.conf

Thanks, now tests are working. I also had issues with my non-US locale. I tried to avoid that and did some changes like SET lc_messages='C', it helps for some cases. If you like, I can add the commit with that to this PR, or as a separate PR. But I was unable to change psql's (client side) messages language, so
finally I had to change my Linux's locale.

So I'll try to fix the rest of your notes. What goes to cleaning the table, I still suggest not to do that. Just write in the import that we are adding records, instead of calling it "import". But if you confirm your decision, I'll implement it.

budden · 2019-02-16T19:22:45Z

Haha, I found this one: https://www.postgresql.org/docs/９.6/sql-copy.html
It allows to feed data via stdin and can read csv format. So it looks like we should not parse csv ourselves, but instead delegate the file contents to the psql via stdin...
I'll experiment with it.

budden · 2019-02-16T19:43:27Z

So something simple as
/y/go/src/github.com/sosedoff/pgweb$ cat /y/a.csv | psql -c "\\copy zzz (text,id) from stdin with (format csv, header true)" does the job. \copy command is advertized to be a counterpart of copy which one can run from psql if having no permissions to call copy. Also one can use just copy, w/o backslashes - it works either.

So one way to go is to simply run psql from stdin. We check that fields match, generate field order so that csv and postgres table match, direct file contents to stdin of psql and spawn psql from pgweb. We can fail however under stupid platforms like ms windows where stdin can be broken sometimes (e.g. fail to support utf-8), or if there are some bugs/limitations related to spawning. So furhter experiments are required. Can you please give a feedback about this approach?

budden · 2019-02-16T21:33:58Z

Fixed all the issues that I was able to understand and which I believe are really issues. The only thing that is missing is tests.

sosedoff · 2019-02-21T00:45:43Z

pkg/api/api.go

+	if utf8.RuneCountInString(fieldDelimiter) != 1 {
+		return '?', errors.New("field delimiter must be a single character (comma is a standard one, CR and LF and 0xFFFD are not allowed)")
+	}
+	delimiter_char, delimiter_char_size := utf8.DecodeRuneInString(fieldDelimiter)


Needs to be camelcase'd

sosedoff · 2019-02-21T00:48:21Z

pkg/api/routes.go

@@ -52,4 +52,5 @@ func SetupRoutes(router *gin.Engine) {
 	api.GET("/history", GetHistory)
 	api.GET("/bookmarks", GetBookmarks)
 	api.GET("/export", DataExport)
+	api.POST("/importCSV", DataImportCSV)


Could you reformat to /import/csv?

sosedoff · 2019-02-21T00:51:25Z

I posted a few more notes. As for COPY with psql - im okay with that approach as long as it works with SSH tunnel (see database dump feature)

budden · 2019-02-21T16:23:43Z

Seem to fix everything. Also added refreshing of table lists and some simple input data validation. Most questionalbe is the change in css files - I removed a fixed alignment for output pane. All seem to be ok at a glance, maybe I overlooked something. Without that change import button didn't work.
Didn't add the test for the entire import. I didn't find an essential api tests so there is no infrastructure, and so far I'm not smart enough to generate one quickly. Any hints? What goes to COPY, I decided not to go into it now - it is just another project.

budden · 2019-02-22T20:56:15Z

Added an integration test. Maybe it is horrible due to lack of understaning of pgweb's globals, but I made it work finally :)

budden · 2019-02-24T11:01:46Z

I dislike this way of testing, but I don't know how to do it better. What is actually done here is a fork of main module. After the fork, application was initialized so that testing scenario has access to both web API (can send requests and get the response), and to the database directly. When testing web api, session stuff is kind of faked, so that there is no login / call api/ logout sequence. IMO this is messy. What I really want to do here is:

create and import the DB
run pgweb as an application from the script (with Exec.Command and pipes)
login into it
call /import/csv api to import data
call /query api
logout
drop db

This way it is a sort of API testing.
However, there are questions. I don't see how to do that with an embedded testing framework, because I need to build pgweb before running the test.

There are options:

i) make the thin layer between main() and app functionality, like func main() { return Main() }. For testing purposes, run Main in a goroutine, and also run a test script in a main thread. Test script only communicates with Main via HTTP and when done sends termination signal.

ii) use Docker. I think that's an overkill

iii) write a separated golang application and use a makefile to run it (we will be unable to use go test)

Your ideas?

sosedoff · 2019-02-24T17:48:18Z

Yeah, tests are kind of tricky due to the way how pgweb is written (this'll change with 1.0 version).
I'd be interested in a test in client_test.go that handles the actual CSV import, don't bother too much with testing the API endpoint. If you're not sure how to proceed - do the full squash of all your changes and i can add the tests.

budden · 2019-02-24T19:01:14Z

Actually I'm in progress to test it via API endpoint and it seems that I'm not very far from obtaining a prettier test. I personally prefer integration tests because they give more value per line of test code :) I hope to finish that today and maybe will do another squashed PR.

budden · 2019-02-24T22:54:58Z

Well, now I'm satisfied with the result. Didn't do squash (too tired today), and recognized that there were no need to export initOptions. Also not sure about removing "absolute" from some

s.

budden · 2019-02-25T18:55:18Z

Superseded by #422

budden added 7 commits February 15, 2019 17:08

Re-applied changes from PR 308

28e6591

Error reporting for CSV import

515ef0f

Make 'Import/Table name' input narrower

d755f48

Horizontal line between import parameters and output

4a1ccf7

Fix showing/higing import

431b52b

Fixed /import request type

8673fdb

Some id changes + Set up the delimiter

27ac8ec

sosedoff reviewed Feb 15, 2019

View reviewed changes

Renamed identifiers to remove "_"

e4f0b18

budden added 3 commits February 17, 2019 00:12

Don't add auto-numbered id column

e0b5d84

Changed error reporting in the DataImportCSV

33b5198

Added a brief explanation about csv import operation

de8b7cb

sosedoff reviewed Feb 21, 2019

View reviewed changes

budden added 8 commits February 21, 2019 14:28

Change snake_case to camelCase, NewTable to QueryInternal

26ec9a9

convert /importCSV route to /import/csv

4255343

Unfix output layout

d7c566d

Reload schemes after import

27ef523

Some client-side error checking

d915778

isPostgresqlIdentifierRequiringNoQuoting

61b9bf5

Fixed isPostgresqlIdentifierRequiringNoQuoting

b21090d

Check that table name and field names require no quoting

f98a70a

budden added 3 commits February 22, 2019 23:10

Export cli.StartServer() and client.Client.LastQueryTime

6d57245

cli_test produced some sound

3a8d433

CSV import integration test seem to work

658cad1

budden added 7 commits February 24, 2019 14:36

Fixed formatting

66aecc3

Naive sleep before accessing server

5c1c428

report teardown error

df3b13b

Another attempt to log teardown error

e1bca66

Yet another place where teardown error is reported

3d0c0c3

fixed formatting

9110f87

attempt to wait before a call to a teardown

dd754d9

budden added 7 commits February 24, 2019 22:52

Reincarnation of the test goes on

0de7515

teardown works

5f3a435

Now test looks satisfactory

d9f4c9c

Fixed "invalid flag" error

edf7ede

Get rid of "auxCloser"

a82636b

Fixed "Rows" layout

c3897c3

Fixed formatting

c80b972

budden added 5 commits February 25, 2019 11:47

Renamed test file

21f637f

Fix main()

5ee756e

Added more appserver tests

4ac346e

Tests improved (but fail)

b57dcb3

Reordered code in tests

183f82f

budden closed this Feb 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSV Import #418

CSV Import #418

budden commented Feb 15, 2019 •

edited

sosedoff commented Feb 15, 2019

sosedoff left a comment

sosedoff Feb 15, 2019

budden Feb 16, 2019

sosedoff Feb 15, 2019

budden Feb 16, 2019

sosedoff Feb 15, 2019

budden Feb 16, 2019

sosedoff Feb 21, 2019

sosedoff Feb 15, 2019 •

edited

budden Feb 16, 2019 •

edited

sosedoff Feb 21, 2019

budden commented Feb 16, 2019 •

edited

budden commented Feb 16, 2019 •

edited

budden commented Feb 16, 2019 •

edited

budden commented Feb 16, 2019 •

edited

budden commented Feb 16, 2019

sosedoff Feb 21, 2019

budden Feb 21, 2019

sosedoff Feb 21, 2019

budden Feb 21, 2019

sosedoff commented Feb 21, 2019

budden commented Feb 21, 2019

budden commented Feb 22, 2019

budden commented Feb 24, 2019

sosedoff commented Feb 24, 2019

budden commented Feb 24, 2019 •

edited

budden commented Feb 24, 2019

budden commented Feb 25, 2019

CSV Import #418

CSV Import #418

Conversation

budden commented Feb 15, 2019 • edited

sosedoff commented Feb 15, 2019

sosedoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sosedoff Feb 15, 2019 • edited

Choose a reason for hiding this comment

budden Feb 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

budden commented Feb 16, 2019 • edited

budden commented Feb 16, 2019 • edited

budden commented Feb 16, 2019 • edited

budden commented Feb 16, 2019 • edited

budden commented Feb 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sosedoff commented Feb 21, 2019

budden commented Feb 21, 2019

budden commented Feb 22, 2019

budden commented Feb 24, 2019

sosedoff commented Feb 24, 2019

budden commented Feb 24, 2019 • edited

budden commented Feb 24, 2019

budden commented Feb 25, 2019

budden commented Feb 15, 2019 •

edited

sosedoff Feb 15, 2019 •

edited

budden Feb 16, 2019 •

edited

budden commented Feb 16, 2019 •

edited

budden commented Feb 16, 2019 •

edited

budden commented Feb 16, 2019 •

edited

budden commented Feb 16, 2019 •

edited

budden commented Feb 24, 2019 •

edited