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

Fix: objectId size for Pointer in Postgres #6619

Merged
merged 3 commits into from
Apr 22, 2020
Merged

Fix: objectId size for Pointer in Postgres #6619

merged 3 commits into from
Apr 22, 2020

Conversation

cbaker6
Copy link
Contributor

@cbaker6 cbaker6 commented Apr 16, 2020

Close #6616

@codecov
Copy link

codecov bot commented Apr 16, 2020

Codecov Report

Merging #6619 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6619   +/-   ##
=======================================
  Coverage   93.91%   93.91%           
=======================================
  Files         169      169           
  Lines       11972    11972           
=======================================
  Hits        11243    11243           
  Misses        729      729           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5ef0be...2f72598. Read the comment docs.

@dplewis
Copy link
Member

dplewis commented Apr 16, 2020

Would this work for existing tables with char(10) for pointers?

@cbaker6
Copy link
Contributor Author

cbaker6 commented Apr 16, 2020

@dplewis I left the test case in for char(10),

objectId: 'qwerty1234', // make it 10 chars to match PG storage

and added a test case for larger. I think most of the tests in the suite uses the standard objectId, so does that mean it will work for existing tables?

@cbaker6
Copy link
Contributor Author

cbaker6 commented Apr 17, 2020

I've did the following tests to confirm the old type of "character(10)" still works the change here:

  • Connect this patched version of parse server to a database that was created using parse server 4.1.0
    -- This database has pointers to the _User class from _Session and other custom classes generated in 4.1.0 and lower
  • Used Parse Dashboard to connect to the patched version
  • Went to some of the classes that pointed to the _User class and clicked on the pointer field in that class, it took me back to the exact _User with the respective objectId (did what it's suppose to do)
  • I also curled the respective class, to make sure it returned the correct pointer
  • This seems to work because from what I see online, char, varchar, and text use the same underlying c function in postgres. char and varchar simply put limitations on the amount of characters where text allows infinite. Some details from the postgres documentation is here
  • Notes
    -- The actual objectId itself is already a text value in the older versions of ParseServer. For some reason, the pointer value to an objectId had the char(10) limitation
    -- The only PostgresStorageAdapter methods that seem to call the new change totext to the pointer are: createTable, addFieldIfNotExists, and updateObjectsByQuery

The potential issue will be if an administrator wants wants to upgrade the objectId's of a table that was created in parse server 4.2.0 or lower to an objectId size greater than 10. In order to do this, they will probably need to do an "ALTER Table..." to change the char(10) to "text" and then go through the respective tables and update the objectId with the new larger one (at least for the pointer values). It seems postgres will need to lock the tables for this change which will cause for some down time on large db's.

Regardless of the change in this PR, an administrator would still have to go through those steps if they started on an earlier version of parse server and want to upgrade to larger objectId sizes

@dplewis
Copy link
Member

dplewis commented Apr 22, 2020

In order to do this, they will probably need to do an "ALTER Table..." to change the char(10) to "text"

This was my main concern with this patch. Either we document the migration steps or add the alter table to the performInitialization function in the PG adapter. But like you said their might be down time.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Apr 22, 2020

@dplewis I think my comment might have been a little confusing. There's no downtime with the patch. People already using 10 character objectId's will still function fine based on the tests I mentioned.

There will be downtime however, if someone wanted to increase their objectId from 10 to a larger value, which, this downtime will have to happen regardless of this patch is added or not, because they will have to convert their objectIds using Alter table.

I used the patch with an older parse-server with no problem, no conversion using Alter table.

char and text seem to use the same underlying c function, so there's no conversion needed. It looks like char(10) was placed there originally for error checking to make sure the objectId was 10, it simply isn't needed.

@dplewis dplewis changed the title Fixing objectId for Pointer in Postgres Fix: objectId size for Pointer in Postgres Apr 22, 2020
@dplewis dplewis merged commit 489aeae into parse-community:master Apr 22, 2020
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

Successfully merging this pull request may close these issues.

ObjectId longer than 10 characters fails for PostgreSQL
2 participants