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

Controllers should not implicitly store data without using the Storage Delegate #16028

Closed
sagar-apple opened this issue Mar 9, 2022 · 1 comment

Comments

@sagar-apple
Copy link
Contributor

sagar-apple commented Mar 9, 2022

Problem

The Storage Delegate passed in to controllers allows the client application to implement storage in any way they see fit.
Unfortunately, our platform layers and the controller facing SDK have divergent mechanisms for storage.

The platform layer(at least on Darwin) uses a build time configured path to decide where things are stored. It has no knowledge of the Controller's storage interface.

This results in any Controller instance storing things in 2 completely separate storages. The Platform Layer's KVS is completely opaque to the client application.

Proposed Solution

#16005 adds a mechanism to configure the platform layer KVS location but that doesn't solve the main problem where 2 disjoint storage implementations are being used.

A controller's storage delegate must override the platform's KVS implementation.

tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Mar 10, 2022
This PR is on the path towards having Server::Server no longer
statically initialize its members with storage, and instead
relying on Server::Init(). This will simplify organization
of unit tests and also the convergence of Controller/Server
storage to address issue project-chip#16028

Issue project-chip#16028
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Mar 10, 2022
- This PR is on the path towards having Server::Server no longer
  statically initialize its members with storage, and instead
  relying on Server::Init(). This will simplify organization
  of unit tests and also the convergence of Controller/Server
  storage to address issue project-chip#16028

Issue project-chip#16028

This PR separates init of AccessControl so that the delegate need not be
passed to the Constructor, which allows simpler splitting of storage
init and decoupling from Server::Server() static inializer list.

This is a structural, non-functional change, touching only AccessControl.

Testing
- Ran cert tests, still pass
- Ran unit tests, still pass
tcarmelveilleux added a commit that referenced this issue Mar 10, 2022
* Allow runtime init of some `Server` members (1/2)

This PR is on the path towards having Server::Server no longer
statically initialize its members with storage, and instead
relying on Server::Init(). This will simplify organization
of unit tests and also the convergence of Controller/Server
storage to address issue #16028

Issue #16028

* Restyled by clang-format

* Move a comment to the correct place

Co-authored-by: Restyled.io <[email protected]>
tcarmelveilleux added a commit that referenced this issue Mar 10, 2022
* Allow runtime init of some `Server` members (2/2)

- This PR is on the path towards having Server::Server no longer
  statically initialize its members with storage, and instead
  relying on Server::Init(). This will simplify organization
  of unit tests and also the convergence of Controller/Server
  storage to address issue #16028

Issue #16028

This PR separates init of AccessControl so that the delegate need not be
passed to the Constructor, which allows simpler splitting of storage
init and decoupling from Server::Server() static inializer list.

This is a structural, non-functional change, touching only AccessControl.

Testing
- Ran cert tests, still pass
- Ran unit tests, still pass

* Restyled by clang-format

* Address review comments

* Restyled by clang-format

* Address review comments to remove mIsInitialized

* Restyled by clang-format

Co-authored-by: Restyled.io <[email protected]>
andrei-menzopol pushed a commit to andrei-menzopol/connectedhomeip that referenced this issue Apr 14, 2022
* Allow runtime init of some `Server` members (1/2)

This PR is on the path towards having Server::Server no longer
statically initialize its members with storage, and instead
relying on Server::Init(). This will simplify organization
of unit tests and also the convergence of Controller/Server
storage to address issue project-chip#16028

Issue project-chip#16028

* Restyled by clang-format

* Move a comment to the correct place

Co-authored-by: Restyled.io <[email protected]>
andrei-menzopol pushed a commit to andrei-menzopol/connectedhomeip that referenced this issue Apr 14, 2022
* Allow runtime init of some `Server` members (2/2)

- This PR is on the path towards having Server::Server no longer
  statically initialize its members with storage, and instead
  relying on Server::Init(). This will simplify organization
  of unit tests and also the convergence of Controller/Server
  storage to address issue project-chip#16028

Issue project-chip#16028

This PR separates init of AccessControl so that the delegate need not be
passed to the Constructor, which allows simpler splitting of storage
init and decoupling from Server::Server() static inializer list.

This is a structural, non-functional change, touching only AccessControl.

Testing
- Ran cert tests, still pass
- Ran unit tests, still pass

* Restyled by clang-format

* Address review comments

* Restyled by clang-format

* Address review comments to remove mIsInitialized

* Restyled by clang-format

Co-authored-by: Restyled.io <[email protected]>
@bzbarsky-apple
Copy link
Contributor

I believe at this point this is fixed. @sagar-apple can you please confirm?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants