Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Datastore - support custom maps #2345

Merged
merged 3 commits into from
May 1, 2020
Merged

Datastore - support custom maps #2345

merged 3 commits into from
May 1, 2020

Conversation

dmitry-s
Copy link
Contributor

fixes #2334

@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #2345 into master will decrease coverage by 7.05%.
The diff coverage is 78.94%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2345      +/-   ##
============================================
- Coverage     81.10%   74.04%   -7.06%     
+ Complexity     2313     2098     -215     
============================================
  Files           259      259              
  Lines          7551     7563      +12     
  Branches        783      785       +2     
============================================
- Hits           6124     5600     -524     
- Misses         1102     1603     +501     
- Partials        325      360      +35     
Flag Coverage Δ Complexity Δ
#integration ? ?
#unittests 74.04% <78.94%> (+0.01%) 2098.00 <3.00> (+1.00)
Impacted Files Coverage Δ Complexity Δ
.../core/convert/DefaultDatastoreEntityConverter.java 87.09% <73.33%> (-6.74%) 25.00 <3.00> (-1.00)
...ta/datastore/core/convert/TwoStepsConversions.java 86.47% <100.00%> (-3.53%) 64.00 <0.00> (-4.00)
...gcp/secretmanager/SecretManagerPropertySource.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-4.00%)
...a/spanner/repository/query/SpannerQueryMethod.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-6.00%)
...retmanager/SecretManagerPropertySourceLocator.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...figure/config/GcpConfigBootstrapConfiguration.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...epository/config/SpannerRepositoriesRegistrar.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
...ository/config/DatastoreRepositoriesRegistrar.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
...restore/GcpFirestoreEmulatorAutoConfiguration.java 0.00% <0.00%> (-84.85%) 0.00% <0.00%> (-4.00%)
...ramework/cloud/gcp/vision/DocumentOcrTemplate.java 17.64% <0.00%> (-73.53%) 4.00% <0.00%> (-5.00%)
... and 53 more

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 e23744d...4c131cf. Read the comment docs.

@ashesfall
Copy link

This is the second merge request resulting from requests I have made. That leads me to wonder if it is possible for me to become a contributor to this project. Is there a process in place?

* @return a Map where the key values are the field names and the values the field
* values.
*/
<T, R> Map<T, R> readAsMap(Class<T> keyType, TypeInformation<R> componentType,
BaseEntity entity);
BaseEntity entity, TypeInformation typeInformation);
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Technically, it's a breaking change. Can we just overload with a new method that has an additional argument?
    This especially seems to make sense because you sometimes call the method with null typeInformation.
  2. Also, can we rename typeInformation to mapTypeInformation?
  3. If you're getting the type information for the whole map, why do you need the key and value type information? Maybe it would be easier if the caller just passed the map they wanted to fill with results?

Object customMap = null;
if (typeInformation != null && !typeInformation.getType().isInterface()) {
try {
customMap = ((Constructor<?>) typeInformation.getType().getConstructor()).newInstance();
Copy link
Contributor

Choose a reason for hiding this comment

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

This just doesn't seem right. Can we put the burden on the caller to create the map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate why it doesn't seem right?

Choose a reason for hiding this comment

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

I think if the caller has to create it, it will perhaps be inconsistent with the other cases.

@meltsufin
Copy link
Contributor

@ashesfall We welcome contributions! All you have to do is open a pull request.
Here's contributor guide.

@dmitry-s dmitry-s requested a review from meltsufin May 1, 2020 16:44
@sonarcloud
Copy link

sonarcloud bot commented May 1, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 3 Code Smells

90.0% 90.0% Coverage
0.0% 0.0% Duplication

@dmitry-s dmitry-s merged commit b01e784 into master May 1, 2020
@dmitry-s dmitry-s deleted the datastore-custom-map branch May 1, 2020 16:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Custom Subclasses of Map Cannot be Stored
3 participants