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

RCORE-2236 Add support for relaxed schema in bind message #7981

Closed

Conversation

michael-wb
Copy link
Contributor

@michael-wb michael-wb commented Aug 12, 2024

What, How & Why?

Adds support for the two new relaxed schema protocol error codes (ErrorClientFileRelaxedSchemaModeChanged (240) and ErrorAppDoesNotSupportRelaxedSchema (241)) and adds the relaxedSchema value to the BIND message JSON data with a value of 1 if the realm file is using a relaxed schema or 0 if it is not.

This PR also bumps the sync protocol version to v15, since there is a protocol bump associated with enabling the relaxed schema feature.

Fixes #7974

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • [ ] C-API, if public C++ API changed
  • [ ] bindgen/spec.yml, if public C++ API changed

@michael-wb michael-wb changed the title Added error codes and BIND JSON flag for relaxed schema RCORE-2236 Add support for relaxed schema in bind message Aug 12, 2024
@michael-wb michael-wb linked an issue Aug 12, 2024 that may be closed by this pull request
Copy link

coveralls-official bot commented Aug 12, 2024

Pull Request Test Coverage Report for Build michael.wilkersonbarker_1334

Details

  • 33 of 45 (73.33%) changed or added relevant lines in 7 files are covered.
  • 56 unchanged lines in 13 files lost coverage.
  • Overall coverage increased (+0.01%) to 91.083%

Changes Missing Coverage Covered Lines Changed/Added Lines %
test/object-store/util/sync/sync_test_utils.cpp 10 14 71.43%
src/realm/sync/protocol.cpp 2 10 20.0%
Files with Coverage Reduction New Missed Lines %
test/fuzz_tester.hpp 1 57.73%
test/test_query2.cpp 1 98.73%
src/realm/mixed.cpp 2 86.46%
src/realm/object-store/shared_realm.cpp 2 91.89%
src/realm/query_expression.hpp 2 93.85%
src/realm/sync/network/http.hpp 2 82.76%
src/realm/sync/noinst/client_impl_base.cpp 2 83.09%
test/test_index_string.cpp 2 93.33%
src/realm/alloc_slab.cpp 4 90.61%
src/realm/link_translator.cpp 4 76.92%
Totals Coverage Status
Change from base Build jorgen.edelbo_383: 0.01%
Covered Lines: 219841
Relevant Lines: 241364

💛 - Coveralls

Copy link
Contributor

@sean-brandenburg sean-brandenburg left a comment

Choose a reason for hiding this comment

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

Nothing blocking, just had a few small comments

return "Client attempted to change their relaxed schema mode for an existing client file - "
"requires client reset";
case ProtocolError::relaxed_schema_not_suppored:
return "Client using a relax schema attempted to connect to an app that does not support "
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit]

Suggested change
return "Client using a relax schema attempted to connect to an app that does not support "
return "Client using relaxed schema mode attempted to connect to an app that does not support "

Comment on lines 66 to 67
// 15 Support for relaxed (flexible) schemas
//
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a problem with doing this early, but worth noting that the server does not yet know about this protocol version (and shouldn't need to for this piece of relaxed schema)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we delay the protocol bump until later? I believe the server will just negotiate to v14 and there is nothing in the sync client or relaxed schema implementation that cares whether it's 14 or 15.

(Also this is merging to a feature branch, so it will be a while before it hits master)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think generally we wait until the project is more or less complete incase some other feature needs to take the earlier protocol version, but I don't feel strongly about waiting since this is a feature branch. Worst case you'll need to increment it by 1 at a later point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted protocol change

Comment on lines 1909 to 1910
// Send 0 if sync client is not using a relaxed schema
bind_json_data["relaxedSchema"] = get_db()->flexible_schema_allowed() ? 1 : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on adding a test that tries to get back one of the new error codes from the server to make sure the server is properly decoding this into a bool (rather than just always defaulting to false or something)?

I don't think this should be an issue, but seems like it would be valuable to at the very least do a manual check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test to validate the "app doesn't support relaxed schema" case - turns out the server doesn't support an int for this value, so it was changed to a bool value.

I recommend updating this to be an int value in case we want to expand the meaning of this value in the future.

@michael-wb michael-wb mentioned this pull request Aug 12, 2024
1 task
@michael-wb michael-wb marked this pull request as draft August 26, 2024 13:01
@jbreams jbreams closed this Sep 18, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for relaxed schema in bind message
4 participants