Skip to content

/dataverse

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

As a developer I'd like some guidance on writing SQL upgrade scripts #5186

Closed
pdurbin opened this Issue Oct 15, 2018 · 14 comments

Comments

Assignees

@kcondon kcondon

Labels
Feature: Code Infrastructure
Projects
None yet
Milestone
  4.10 - Additional...
7 participants
@pdurbin @mheppler @qqmyers @scolapasta @kcondon @matthew-a-dunlap @djbrooke
@pdurbin
Copy link
Member

pdurbin commented Oct 15, 2018

I just switched to the "develop" branch and could not deploy due to this error:

Caused by: Exception [EclipseLink-4002] (Eclipse Persistence Services - 2.5.2.v20140319-9ad6abd): org.eclipse.persistence.exceptions.DatabaseException
Internal Exception: org.postgresql.util.PSQLException: ERROR: column "uri" does not exist
Position: 184
Error Code: 0
Call: SELECT ID, ADVANCEDSEARCHFIELDTYPE, ALLOWCONTROLLEDVOCABULARY, ALLOWMULTIPLES, description, DISPLAYFORMAT, DISPLAYONCREATE, DISPLAYORDER, FACETABLE, FIELDTYPE, name, REQUIRED, title, uri, VALIDATIONFORMAT, WATERMARK, METADATABLOCK_ID, PARENTDATASETFIELDTYPE_ID FROM DATASETFIELDTYPE ORDER BY ID
Query: ReadAllQuery(referenceClass=DatasetFieldType sql="SELECT ID, ADVANCEDSEARCHFIELDTYPE, ALLOWCONTROLLEDVOCABULARY, ALLOWMULTIPLES, description, DISPLAYFORMAT, DISPLAYONCREATE, DISPLAYORDER, FACETABLE, FIELDTYPE, name, REQUIRED, title, uri, VALIDATIONFORMAT, WATERMARK, METADATABLOCK_ID, PARENTDATASETFIELDTYPE_ID FROM DATASETFIELDTYPE ORDER BY ID")

It looks like the "uri" field was added to the "datasetfieldtype" table in 319c8d9 as part of pull request #5047 which was merged on Friday afternoon.

As of e948af2 scripts/database/upgrades/upgrade_v4.9.4_to_v4.10.sql on the "develop" branch does not contain any SQL statements to add the new "uri" column so the definition of done for this issue is:

We could expand the scope slightly to say that the dev guide should explain the procedure above. Perhaps "database migrations" deserves its own page? We should probably also mention the need to write SQL update scripts and let others know about them in http://guides.dataverse.org/en/4.9.4/developers/version-control.html#how-to-make-a-pull-request

@pdurbin pdurbin added the Status: This/Next Sprint label Oct 15, 2018

@mheppler

This comment has been minimized.

Sign in to view
Copy link
Contributor

mheppler commented Oct 15, 2018 •
edited

I just ran into this myself locally. Found the necessary script from @qqmyers here 50fea8f.

ALTER TABLE datasetfieldtype ADD COLUMN uri text;
ALTER TABLE metadatablock ADD COLUMN namespaceuri text;
@pdurbin

This comment has been minimized.

Sign in to view
Copy link
Member Author

pdurbin commented Oct 15, 2018

@mheppler oh! It's called "scripts/database/5043-update.sql". Thanks!

Is this the new way? Shall we document it in the dev guide? And explain the new way in a post to the "dataverse-dev" list?
@qqmyers

This comment has been minimized.

Sign in to view
Copy link
Member

qqmyers commented Oct 15, 2018

@pdurbin I think I just put it in that way since I didn't know what version it would become part of... If I should have done it differently, let me know...
@mheppler

This comment has been minimized.

Sign in to view
Copy link
Contributor

mheppler commented Oct 15, 2018

That's a very valid point @qqmyers and something we should consider moving forward, especially as we start ramping up the speed of releases with #4980.
@pdurbin

This comment has been minimized.

Sign in to view
Copy link
Member Author

pdurbin commented Oct 15, 2018

@qqmyers here's what I do:

  • Look in scripts/database/upgrades to see if there's a script that's already been merged that indicates what the next release will be. If it exists, add my SQL updates statements to it.
  • If there is no SQL script in scripts/database/upgrades that takes us to the current released version to the next planned release, double check with @djbrooke what the next release number will be and then create a script with my updates.
  • Wait for my pull request to get merged and then let other developers know how to keep their dev environments working by sending an email to "dataverse-dev" like the one @sekmiller just sent: https://groups.google.com/d/msg/dataverse-dev/Vzfl3IxpTww/AutZIZRCBwAJ

Does that make sense? I could make a pull request to document this.

Are you interested in making a pull request to remove scripts/database/5043-update.sql and put the statements in scripts/database/upgrades/upgrade_v4.9.4_to_v4.10.sql? I'm happy to do this if you want.

Are you interested in emailing dataverse-dev about the update? I can do this too.

Finally, since I'm bothering you, can you please take a quick peek at #5185? Thanks!!

@qqmyers qqmyers referenced this issue Oct 15, 2018

Merged

provide guidance on running SQL upgrade scripts and do a little reorg #5189

0 of 5 tasks complete

@pdurbin pdurbin added Status: Code Review and removed Status: This/Next Sprint labels Oct 15, 2018

@pdurbin

This comment has been minimized.

Sign in to view
Copy link
Member Author

pdurbin commented Oct 15, 2018

@qqmyers pull request #5189 looks good for moving the SQL statements to the right place. Do you want me to add a bit about our process to the dev guide? (Some of the "here's what I do" above.) We could also defer messing with the dev guide.

@djbrooke djbrooke assigned pdurbin Oct 15, 2018

@scolapasta

This comment has been minimized.

Sign in to view
Copy link
Contributor

scolapasta commented Oct 15, 2018

So to weigh in:
For a while I've been advocating we take this approach (naming db scripts based on issue and then compiling them all at the time we create a release). Too many times we've had to rename db scripts because we change our plan of what is next.

we will discuss it here as a team and can provide some guidance going forward.

@pdurbin pdurbin assigned scolapasta and unassigned pdurbin Oct 15, 2018

@pdurbin

This comment has been minimized.

Sign in to view
Copy link
Member Author

pdurbin commented Oct 15, 2018

@scolapasta makes sense. I'm assigning this to you to sort out. As long as we document the process. Thanks!
@kcondon

This comment has been minimized.

Sign in to view
Copy link
Contributor

kcondon commented Oct 15, 2018

@pdurbin So does this ticket cover actually renaming/ integrating this script into the update one then or is it more of a process discussion?
@pdurbin

This comment has been minimized.

Sign in to view
Copy link
Member Author

pdurbin commented Oct 15, 2018

@kcondon good question. Without a process I'm blocked on making SQL updates. I'm attempting to use the new process in pull request #5195 but as @scolapasta indicated we will be discussing this as a team.
@scolapasta

This comment has been minimized.

Sign in to view
Copy link
Contributor

scolapasta commented Oct 15, 2018

For this issue, we can use the current process of using the update script, if people prefer, so we can get it done and over. The more general discussion will happen in the next few weeks, I hope.

@scolapasta scolapasta removed their assignment Oct 15, 2018

@pdurbin

This comment has been minimized.

Sign in to view
Copy link
Member Author

pdurbin commented Oct 15, 2018

Ok, I'll document the process we've been using since I got here.

@pdurbin pdurbin self-assigned this Oct 15, 2018

pdurbin added a commit to QualitativeDataRepository/dataverse that referenced this issue Oct 16, 2018

@pdurbin
avoid confusion by removing 3561-update.sql IQSS#5186 
This script is referenced from
https://github.com/IQSS/dataverse/releases/tag/v4.8 and it's unclear
why it was not part of upgrade_v4.7.1_to_v4.8.sql

New contributors have found the presence of this script in the
"scripts/database" directory confusing and have assumed that the process
they should follow is to create a similar script, not knowing about our
upgrade_v4.7.1_to_v4.8.sql process.
7c67734
@pdurbin

This comment has been minimized.

Sign in to view
Copy link
Member Author

pdurbin commented Oct 16, 2018

As discussed at standup this morning I'll be adding some documentation of our existing process for writing SQL upgrade script. I'll tweak the title of this issue to reflect this.

At standup we also agreed to remove scripts/database/3561-update.sql which leads contributors astray into thinking that's what we want. I did this in 7c67734 and had the thought that we could move the content into upgrade_v4.7.1_to_v4.8.sql and update the release notes at https://github.com/IQSS/dataverse/releases/tag/v4.8 but didn't want to make this decision unilaterally.

@pdurbin pdurbin changed the title PSQLException: ERROR: column "uri" does not exist on DatasetFieldType As a developer I'd like some guidance on writing SQL upgrade scripts Oct 16, 2018

@pdurbin pdurbin added Status: Development and removed Status: Code Review labels Oct 16, 2018

pdurbin added a commit to QualitativeDataRepository/dataverse that referenced this issue Oct 16, 2018

@pdurbin
add SQL Upgrade Scripts section to Dev Guide IQSS#5186
b802c85

@pdurbin pdurbin added Status: Code Review and removed Status: Development labels Oct 16, 2018

@pdurbin pdurbin removed their assignment Oct 16, 2018

@pdurbin

This comment has been minimized.

Sign in to view
Copy link
Member Author

pdurbin commented Oct 16, 2018

I just added at "SQL Upgrade Scripts" section to the Dev Guide in b802c85

@qqmyers et al. you are welcome to give feedback!

@pdurbin pdurbin added the Feature: Code Infrastructure label Oct 17, 2018

@djbrooke djbrooke assigned matthew-a-dunlap Oct 17, 2018

@matthew-a-dunlap matthew-a-dunlap added Status: QA and removed Status: Code Review labels Oct 19, 2018

@matthew-a-dunlap matthew-a-dunlap removed their assignment Oct 19, 2018

@kcondon kcondon self-assigned this Oct 19, 2018

@kcondon kcondon closed this Oct 19, 2018

@kcondon kcondon removed the Status: QA label Oct 19, 2018

@djbrooke djbrooke added this to the 4.10 - Additional Data Transfer Options milestone Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.