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

Backwards compatibility with older style structured properties. #126

Merged

Conversation

chrisrossi
Copy link
Contributor

When implementing structured properties the first time, I just used
Datastore's native embedded entity functionality, not realizing that NDB
had originally used dotted property names instead. (Probably GAE
Datastore didn't have embedded entities when NDB was originally
written.) The problem is that users migrating from GAE NDB can't load
entities with structured properties from their existing datastore. This
PR makes NDB backwards compatible with older, dotted name style
structured properties so that existing repositories still work with the
new NDB.

Fixes #122.

@chrisrossi chrisrossi requested a review from cguardia June 27, 2019 17:17
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 27, 2019
@chrisrossi
Copy link
Contributor Author

Note that coverage is complaining about an uncovered branch, but it looks to me that it is covered, so I'm not sure what's going on there. Still trying to figure that one out.

Copy link
Contributor

@cguardia cguardia left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

else:
value.b_val.update({subname: subvalue})

continue
Copy link
Contributor

Choose a reason for hiding this comment

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

I've had trouble with coverage in continue statements before. Seems like the python optimizer just skips that and goes to the start of the loop directly, so the line is never reached. Solutions are: 1) Use "# pragma: no branch". 2) Turn off the optimizer. I have gone with 1) when I encounter this problem.

for name, value in ds_entity.items():
prop = getattr(model_class, name, None)

# Backwards compatibility shim. NDB previously stored structured
# properties as sets of dotted name proprties. Datastore now has native
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo on "proprties", but the just merged spell checker should catch this before your next push (once you rebase, of course).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it looks like spell checker doesn't find it because comments don't make it into docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha, I thought it was in a doc string. Duh.

Copy link
Contributor

@cguardia cguardia left a comment

Choose a reason for hiding this comment

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

Forgot to approve.

When implementing structured properties the first time, I just used
Datastore's native embedded entity functionality, not realizing that NDB
had originally used dotted property names instead. (Probably GAE
Datastore didn't have embedded entities when NDB was originally
written.) The problem is that users migrating from GAE NDB can't load
entities with structured properties from their existing datastore. This
PR makes NDB backwards compatible with older, dotted name style
structured properties so that existing repositories still work with the
new NDB.

Fixes googleapis#122.
@chrisrossi
Copy link
Contributor Author

Filed a bug for the coverage thing:
nedbat/coveragepy#817

@chrisrossi chrisrossi force-pushed the fix-legacy-structured-properties branch from 8f94e99 to 83869fa Compare June 28, 2019 14:49
@alexrwix
Copy link
Contributor

Hi Chris!
I tried your latest changes with our existing content. All my tests are passing.
It seems to be working fine now.
Thank you for the quick fix!

@chrisrossi chrisrossi merged commit 23e82d7 into googleapis:master Jun 28, 2019
@chrisrossi chrisrossi deleted the fix-legacy-structured-properties branch June 28, 2019 14:59
@chrisrossi
Copy link
Contributor Author

Hi Chris!
I tried your latest changes with our existing content. All my tests are passing.
It seems to be working fine now.
Thank you for the quick fix!

No problem. Thanks for the bug report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StructuredProperty backward compatibility issue
4 participants