Fix failing test cases for proto1 -> proto2 migration#291
Merged
srinjoyray merged 21 commits intoproto2from Nov 9, 2024
Merged
Fix failing test cases for proto1 -> proto2 migration#291srinjoyray merged 21 commits intoproto2from
srinjoyray merged 21 commits intoproto2from
Conversation
ludoch
reviewed
Oct 28, 2024
| DatastoreApiHelper.makeAsyncCall( | ||
| apiConfig, DatastoreService_3.Method.BeginTransaction, request.build(), remoteTxn.build()); | ||
| apiConfig, DatastoreService_3.Method.BeginTransaction, request.build(), remoteTxn.buildPartial()); | ||
|
|
Collaborator
There was a problem hiding this comment.
I am still not sure we need buildPartial() in api (thinking only in test code, not code we give to customers).
So are you sure these build to buildPartial really solve test faitures?
Maybe we could change some (non public API) to use builders as long as possible, until we really need to build it)
api/src/main/java/com/google/appengine/api/datastore/DataTypeTranslator.java
Show resolved
Hide resolved
api/src/main/java/com/google/appengine/api/datastore/KeyTranslator.java
Outdated
Show resolved
Hide resolved
| for(Element element : elements){ | ||
| reversedPath.addElement(element); | ||
| } | ||
| reference.setPath(reversedPath.build()); |
Collaborator
There was a problem hiding this comment.
Is there a setPathBuilder instead? Not sure.
Contributor
Author
There was a problem hiding this comment.
setPathBuilder is not present
api_dev/src/test/java/com/google/appengine/api/datastore/DatastoreApiHelperTest.java
Outdated
Show resolved
Hide resolved
| // If the value is indexed it appears in queries, but distinction between | ||
| // null and empty list is lost. | ||
| } | ||
| property.setValue(PropertyValue.getDefaultInstance()); |
Collaborator
There was a problem hiding this comment.
Might want help from datastore team there. I am worried about the comment "// Backward compatible behavior", the logic could break moving to proto2... Also, I do not fully understand the logic of setting a default value, while the method arguments above take a collections of values.
Contributor
Author
There was a problem hiding this comment.
Since value is one of the required fields, not setting it is causing build() to fail
|
|
||
| try { | ||
| responseProto.getParserForType().parseFrom(responseBytes); | ||
| updatedResponseProto = (T) responseProto.getParserForType().parseFrom(responseBytes); |
Collaborator
There was a problem hiding this comment.
Can you explain why we need a new variable?
Contributor
Author
There was a problem hiding this comment.
I am trying to pass the response argument as a builder in this func, then we would not need another variable. But facing a bit of difficulty in handling the other functions which calls this makeAsyncCall method
|
|
||
| public static Index convertFromPb(OnestoreEntity.Index index) { | ||
| return convertFromPb(OnestoreEntity.CompositeIndex.newBuilder().setId(0).setDefinition(index).build()); | ||
| return convertFromPb(OnestoreEntity.CompositeIndex.newBuilder().setId(0).setDefinition(index).setAppId("").setState( |
Collaborator
There was a problem hiding this comment.
Hum, sure an emtpy appId is fine there? Maybe, but not sure!
Contributor
Author
There was a problem hiding this comment.
I'm using the values that are present in the private default constructor
| .setName(predicate.getPropertyName()) | ||
| .setValue(DataTypeTranslator.toV3Value(value)); | ||
| .setValue(DataTypeTranslator.toV3Value(value)) | ||
| .setMultiple(false); |
Collaborator
There was a problem hiding this comment.
Safe to be false? Add a comment.
api_dev/src/test/java/com/google/appengine/api/datastore/CursorTest.java
Outdated
Show resolved
Hide resolved
* Copybara import of the project: -- bbad663 by Mend Renovate <bot@renovateapp.com>: Update all non-major dependencies COPYBARA_INTEGRATE_REVIEW=#285 from renovate-bot:renovate/all-minor-patch bbad663 PiperOrigin-RevId: 684477251 Change-Id: If7dfb2beb25d1378618a884350ec5d8d4bd3dd46 * Copybara import of the project: -- af7d544 by Mend Renovate <bot@renovateapp.com>: Update all non-major dependencies COPYBARA_INTEGRATE_REVIEW=#286 from renovate-bot:renovate/all-minor-patch af7d544 PiperOrigin-RevId: 685680510 Change-Id: I96df12dbd2fb7c6eb6cfaae3489626a14766bef9 * Upgrade GAE Java version from 2.0.30 to 2.0.31 and prepare next version 2.0.32-SNAPSHOT PiperOrigin-RevId: 686867662 Change-Id: Id02da08eb160552da419cace43b86c3558e08b0d * Update all non-major dependencies * Update all non-major dependencies * Fix multithread build by ensuring jars are build correctly before copy those jars Signed-off-by: Olivier Lamy <olamy@apache.org> * Update dependency com.google.cloud:google-cloud-logging to v3.20.5 * Simplity copying dependencies Signed-off-by: Olivier Lamy <olamy@apache.org> * exclude not needed artifacts Signed-off-by: Olivier Lamy <olamy@apache.org> * Do not include runtime-* in WEB-INF/lib Signed-off-by: Olivier Lamy <olamy@apache.org> * Copybara import of the project: -- cb49a97 by Mend Renovate <bot@renovateapp.com>: Update all non-major dependencies COPYBARA_INTEGRATE_REVIEW=#299 from renovate-bot:renovate/all-minor-patch cb49a97 PiperOrigin-RevId: 690485840 Change-Id: I76205310268bc27e3c1ab56be4d42b6d486f4e4c * Copybara import of the project: -- 05d0d3c by Mend Renovate <bot@renovateapp.com>: Update all non-major dependencies COPYBARA_INTEGRATE_REVIEW=#299 from renovate-bot:renovate/all-minor-patch 05d0d3c PiperOrigin-RevId: 690870869 Change-Id: I1ba450e60d8637dfd64adb866a5b185b08a744ad * Update all non-major dependencies --------- Signed-off-by: Olivier Lamy <olamy@apache.org> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Srinjoy Ray <srinjoyray@google.com> Co-authored-by: GAE Java Team <gae-java-bot@google.com> Co-authored-by: Olivier Lamy <olamy@apache.org>
* Copybara import of the project: -- bbad663 by Mend Renovate <bot@renovateapp.com>: Update all non-major dependencies COPYBARA_INTEGRATE_REVIEW=#285 from renovate-bot:renovate/all-minor-patch bbad663 PiperOrigin-RevId: 684477251 Change-Id: If7dfb2beb25d1378618a884350ec5d8d4bd3dd46 * Copybara import of the project: -- af7d544 by Mend Renovate <bot@renovateapp.com>: Update all non-major dependencies COPYBARA_INTEGRATE_REVIEW=#286 from renovate-bot:renovate/all-minor-patch af7d544 PiperOrigin-RevId: 685680510 Change-Id: I96df12dbd2fb7c6eb6cfaae3489626a14766bef9 * Upgrade GAE Java version from 2.0.30 to 2.0.31 and prepare next version 2.0.32-SNAPSHOT PiperOrigin-RevId: 686867662 Change-Id: Id02da08eb160552da419cace43b86c3558e08b0d * Update all non-major dependencies * Update all non-major dependencies * Fix multithread build by ensuring jars are build correctly before copy those jars Signed-off-by: Olivier Lamy <olamy@apache.org> * Update dependency com.google.cloud:google-cloud-logging to v3.20.5 * Simplity copying dependencies Signed-off-by: Olivier Lamy <olamy@apache.org> * exclude not needed artifacts Signed-off-by: Olivier Lamy <olamy@apache.org> * Do not include runtime-* in WEB-INF/lib Signed-off-by: Olivier Lamy <olamy@apache.org> * Copybara import of the project: -- cb49a97 by Mend Renovate <bot@renovateapp.com>: Update all non-major dependencies COPYBARA_INTEGRATE_REVIEW=#299 from renovate-bot:renovate/all-minor-patch cb49a97 PiperOrigin-RevId: 690485840 Change-Id: I76205310268bc27e3c1ab56be4d42b6d486f4e4c * Copybara import of the project: -- 05d0d3c by Mend Renovate <bot@renovateapp.com>: Update all non-major dependencies COPYBARA_INTEGRATE_REVIEW=#299 from renovate-bot:renovate/all-minor-patch 05d0d3c PiperOrigin-RevId: 690870869 Change-Id: I1ba450e60d8637dfd64adb866a5b185b08a744ad * Update all non-major dependencies --------- Signed-off-by: Olivier Lamy <olamy@apache.org> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Srinjoy Ray <srinjoyray@google.com> Co-authored-by: GAE Java Team <gae-java-bot@google.com> Co-authored-by: Olivier Lamy <olamy@apache.org>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.