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.

Sign up for GitHub

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

Jump to bottom

fix createJobTable failure when table name contains dot #53

Merged
merged 4 commits into from
Oct 31, 2020
Merged

fix createJobTable failure when table name contains dot #53

merged 4 commits into from
Oct 31, 2020

Conversation

kanagarajkm
Copy link
Contributor

createJobTable fails when the table name contains a dot
for example myschema.jobs.

Fixes #52

@saurabhnanda
Copy link
Owner

@kanagarajkm good catch. But I think the correct fix is to use Identifier everywhere and let postgresql-simple do the rest. Would you mind taking that approach with this PR?

@kanagarajkm
Copy link
Contributor Author

@saurabhnanda yes, i can do that. But i am unsure about how to use PGS.Identifier in createNotificationTrigger function. Could you please help me with an example?

@kanagarajkm kanagarajkm force-pushed the fix-table-name branch 2 times, most recently from bfd1478 to 76a1fd0 Compare October 13, 2020 06:28
@saurabhnanda
Copy link
Owner

@kanagarajkm Try doing the following

  • Change type TableName = PGS.Identifier
  • Change the way cfgTableName is being "spliced" into the queries. Here's an example:
let t :: Identifier = "public.jobs"
formatQuery conn "select * from ? where id=?" (t, 1 :: Int)

@kanagarajkm
Copy link
Contributor Author

@saurabhnanda Thanks for the suggestion. I will update the PR accordingly.

@kanagarajkm
fix createJobTable failure when table name contains dot
4bc3c82 
`createJobTable` fails when the table name contains a dot
for example `myschema.jobs`.
@kanagarajkm
Copy link
Contributor Author

@saurabhnanda i have updated the PR, please take a look.

mrputty
mrputty reviewed Oct 14, 2020
View reviewed changes
src/OddJobs/Types.hs Outdated
@@ -33,10 +34,10 @@ import Control.Monad.Logger (LogLevel)
-- myJobsTable :: TableName
-- myJobsTable = "my_jobs"
-- @
type TableName = PGS.Query
type TableName = PGS.Identifier
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment on this type should be updated, as it refers to the old underlying Query type. Perhaps it could be phrased in a way that says what it is for, rather than what its type is, so that it can't get out of sync with the code. Something like "A type alias used for table names. ..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@mrputty mrputty mentioned this pull request Oct 18, 2020
Draft
@kanagarajkm
Copy link
Contributor Author

@saurabhnanda when Identifier is used for the table name, i see the tables are created with double quotes on postgres. For example, if the table name is myschema.jobs, they are created with the name "myschema.jobs". I am not sure if this will create problems for existing tables. Could you please take a look and confirm?

@saurabhnanda
Copy link
Owner

when Identifier is used for the table name, i see the tables are created with double quotes on postgres. For example, if the table name is myschema.jobs, they are created with the name "myschema.jobs". I am not sure if this will create problems for existing tables. Could you please take a look and confirm?

@kanagarajkm this doesn't seem right at all. This seems like a bug in postgresql-simple. Would you like to open a bug in that repo and see where this goes? Or are we using Identifier incorrectly?

@kanagarajkm
Copy link
Contributor Author

@kanagarajkm this doesn't seem right at all. This seems like a bug in postgresql-simple. Would you like to open a bug in that repo and see where this goes? Or are we using Identifier incorrectly?

@saurabhnanda switched to QualifiedIdentifier which seems to be appropriate here. Now i see the table is created as expected. It should work fine for the existing tables as well. Please take a look.

@saurabhnanda saurabhnanda merged commit 6208bb2 into saurabhnanda:master Oct 31, 2020
@saurabhnanda
Copy link
Owner

Thanks @kanagarajkm -- merged it just in time for Hacktoberfest. Sorry about the delay -- have had a crazy week at work.

@kanagarajkm
Copy link
Contributor Author

kanagarajkm commented Oct 31, 2020 •
edited
Loading

@saurabhnanda thanks a lot!. Awesome work on the library.

@kanagarajkm kanagarajkm deleted the fix-table-name branch October 31, 2020 16:31
@kanagarajkm
Copy link
Contributor Author

@saurabhnanda is there a plan to make a new release with the latest fixes? I am using the github repo as the dependency on our project. Thanks.

@saurabhnanda
Copy link
Owner

@kanagarajkm I was waiting to make a new release to bundle all breaking changes together (especially #55). Are you in a hurry to get a public release?

saurabhnanda added a commit that referenced this pull request Nov 12, 2020
@kanagarajkm @saurabhnanda
Support qualified table names (i.e. [schame_name].[table_name]) (#53)
a7e8870 
* fix createJobTable failure when table name contains dot

`createJobTable` fails when the table name contains a dot
for example `myschema.jobs`.

* update TableName comment

* use QualifiedIdentifier

* Modified docs for TableName

Co-authored-by: Saurabh Nanda <[email protected]>
@kanagarajkm
Copy link
Contributor Author

@saurabhnanda ok, its not urgent, i am using the git source for now. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrputty mrputty mrputty left review comments

@saurabhnanda saurabhnanda saurabhnanda approved these changes

Assignees
No one assigned
Labels
hacktoberfest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

createJobTable fails when the table name has a dot
3 participants
@kanagarajkm @saurabhnanda @mrputty