[#11001] Migrate App Engine Text datastore type #11019
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @jianhandev, these parts of your pull request do not appear to follow our contributing guidelines:
|
jianhandev
changed the title
Derek-Hardy
reviewed
Mar 8, 2021
| } | ||
|
|
||
| public void setInstructorPrivilegeAsText(String instructorPrivilegesAsText) { | ||
| this.instructorPrivilegesAsText = new Text(instructorPrivilegesAsText); | ||
| this.instructorPrivilegesAsText = new StringValue(instructorPrivilegesAsText); |
There was a problem hiding this comment.
Probably you need a null check here?
e.g.
if (instructorPrivilegesAsText == null) {
this.instructorPrivilegesAsText = null;
} else {
this.instructorPrivilegesAsText = new StringValue(instructorPrivilegesAsText);
}There was a problem hiding this comment.
Right, StringValue unlike Text does not support null value. I was thinking can construct StringValue using an empty string instead whenever instructorPrivilegesAsText is null, but this still results in many test failures. Also considering the use of String type as suggested by @wkurniawan07 without indexing, perhaps that would be a better solution.
wkurniawan07
reviewed
Mar 8, 2021
| @@ -52,7 +52,7 @@ | |||
| @Unindex | |||
| private String displayedName; | |||
|
|
|||
| private Text instructorPrivilegesAsText; | |||
| private StringValue instructorPrivilegesAsText; | |||
There was a problem hiding this comment.
It was implicitly unindexed as the type was Text, but best to just add @Unindex here.
There was a problem hiding this comment.
I was pondering over this for a while too! I read that for text strings longer than 1500 bytes (which are not indexed), we should use a Text instance but it is no longer available now. @Unindex can be added to overcome the 1500 bytes limit.
Derek-Hardy
added
s.FinalReview
wkurniawan07
requested changes
Mar 9, 2021
| } | ||
|
|
||
| public void setQuestionText(String questionText) { | ||
| this.questionText = questionText == null ? null : new Text(questionText); | ||
| this.questionText = questionText == null ? null : questionText; |
There was a problem hiding this comment.
For all these getters and setter you can just return or set as is, because there is no object wrapper anymore
| @@ -52,7 +51,8 @@ | |||
| @Unindex | |||
| private String displayedName; | |||
|
|
|||
| private Text instructorPrivilegesAsText; | |||
| @Unindex | |||
| private String instructorPrivileges; | |||
There was a problem hiding this comment.
This is backward-incompatible and will require data migration. Let's not change the field name.
jianhandev
force-pushed
the
11001-remove-appengine-api-datastore-usage
branch
from
a3e6934
to
7631b94
Compare
…tastore Text type
jianhandev
changed the title
wkurniawan07
approved these changes
Mar 10, 2021
Derek-Hardy
approved these changes
Mar 11, 2021
Derek-Hardy
added
s.ToMerge
wkurniawan07
pushed a commit
that referenced
this pull request
Mar 22, 2021
* Replace usage of Text type from appengine datastore api with StringValue * Use unindexed String as alternative over StringValue previously used to replace Text * Simplify getters and setters * Remove redundant architecture test method after deprecating use of datastore Text type
wkurniawan07
pushed a commit
that referenced
this pull request
Mar 22, 2021
* Replace usage of Text type from appengine datastore api with StringValue * Use unindexed String as alternative over StringValue previously used to replace Text * Simplify getters and setters * Remove redundant architecture test method after deprecating use of datastore Text type
wkurniawan07
pushed a commit
that referenced
this pull request
Mar 23, 2021
* Replace usage of Text type from appengine datastore api with StringValue * Use unindexed String as alternative over StringValue previously used to replace Text * Simplify getters and setters * Remove redundant architecture test method after deprecating use of datastore Text type
wkurniawan07
pushed a commit
that referenced
this pull request
Apr 28, 2021
* Replace usage of Text type from appengine datastore api with StringValue * Use unindexed String as alternative over StringValue previously used to replace Text * Simplify getters and setters * Remove redundant architecture test method after deprecating use of datastore Text type
wkurniawan07
pushed a commit
that referenced
this pull request
May 3, 2021
* Replace usage of Text type from appengine datastore api with StringValue * Use unindexed String as alternative over StringValue previously used to replace Text * Simplify getters and setters * Remove redundant architecture test method after deprecating use of datastore Text type
wkurniawan07
pushed a commit
that referenced
this pull request
Jun 2, 2021
* Replace usage of Text type from appengine datastore api with StringValue * Use unindexed String as alternative over StringValue previously used to replace Text * Simplify getters and setters * Remove redundant architecture test method after deprecating use of datastore Text type
wkurniawan07
pushed a commit
that referenced
this pull request
Jun 6, 2021
* Replace usage of Text type from appengine datastore api with StringValue * Use unindexed String as alternative over StringValue previously used to replace Text * Simplify getters and setters * Remove redundant architecture test method after deprecating use of datastore Text type
wkurniawan07
added
the
c.Task
wkurniawan07
pushed a commit
that referenced
this pull request
Jun 6, 2021
* Replace usage of Text type from appengine datastore api with StringValue * Use unindexed String as alternative over StringValue previously used to replace Text * Simplify getters and setters * Remove redundant architecture test method after deprecating use of datastore Text type
daongochieu2810
pushed a commit
to daongochieu2810/teammates
that referenced
this pull request
Jul 5, 2021
…1019) * Replace usage of Text type from appengine datastore api with StringValue * Use unindexed String as alternative over StringValue previously used to replace Text * Simplify getters and setters * Remove redundant architecture test method after deprecating use of datastore Text type
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #11001
This PR cleans up existing code which still contain traces using the old
com.google.appengine.apiused before the Objectify 6 migration.In Objectify 6, there's no way to explicitly store a
Texttype with the Google-provided SDK. The alternative,StringValuedoes not supportnull.Texttype from thecom.google.appengine.api.datastorepackage used in Objectify 5 with unindexedString.