Skip to content
This repository has been archived by the owner on May 17, 2021. It is now read-only.

Added CRUDPersistenceService class #24

Closed
wants to merge 1 commit into from

Conversation

cdjackson
Copy link
Contributor

This adds a new persistence service class (CRUDPersistenceService). It extends QueryablePersistenceService so will work fine with all existing code that uses this class.

The class adds 3 new methods -:
update - to update a record in the store
delete - to delete a record in the store
getRecordCount - to retrieve the number of records for a particular item

The use case is to base the new mysql (and postgres) service on this class to allow additional features to be added via the REST interface to allow removal/modification of records if necessary (since rogue numbers do happen, and these can completely mess with a dataset)

@cdjackson
Copy link
Contributor Author

@kaikreuzer Hi Kai. I was just wondering what your thoughts were with this since you haven't merged it. I'm quite keen to get this merged so I can get the mysql service submitted as well (so that the postgres service can be finished...).

If there's a problem with this class please let me know so we can resolve it, or if you don't think these features should be included in openHAB, I'll revert the mysql service to extend from the QueryablePersistenceService. However I think this would be a backwards step since update and delete are I believe necessary methods for managing the persistence properly and I think that this should be included in openHAB rather than requiring people to use other resources to manage the data. The getRecordCount could be removed if you're not happy with that method, but I think it's useful for paging to know how many records are available.

If you could let me know your thoughts, that would be great - I appreciate you're busy...

Thanks
Chris

@kaikreuzer
Copy link
Member

Sorry for not having merged it yet - the reason is simple: I need to have a closer look at it and I am currently on holiday with very limited time and a terrible internet connection :-)
Will come back to you in a few days (latest next week) - promised!

@cdjackson
Copy link
Contributor Author

No problem - thanks Kai. So long as it's still under consideration that's fine.

Sorry to hassle you on your holiday - I thought from all the emails that you must have been back, so you're doing a pretty good job keeping up :)

Cheers

Chris

 

@kaikreuzer
Copy link
Member

Thinking about it, I am a bit reluctant to add this interface. So far the persistence services were meant to store time series of data - they react on events (or periodically) and store things as a determined fact. Adding the possibility to update or remove values is like permitting forgery - it is somehow against he idea of time series. A valid use case that I see is to truncate certain data (because it is either too old or because there are too many entries altogether) - but for this there could be a dedicated method instead of a generic "delete".
I didn't yet look into the details of HABmin and the changes to the REST API - maybe I am missing some important use case here, so please help me further understanding it :-)

@cdjackson
Copy link
Contributor Author

they react on events (or periodically) and store things as a determined fact

 
Hmmm - determined fact :) There's your problem... I've go a load of wireless sensors - the error detection should keep the facts, factual, but sometimes not...

 
My main use case for this is to remove items from the persistence store that are "bogus". I've got data going back a few years (on my Vera still at the moment) and every now and then you get an energy value that's off the charts (maybe 65k watts or something). This completely screws up the repository - if you graph it out, you end up with a big spike, and not really seeing the "real" data. Somehow, you need to go in and edit this point out otherwise it messes with stats. I think that this editing should be done through a single, consistent interface, rather than through the backdoor (ie. direct into the database which is the other alternative, but I think this would really be a shame).

 
Personally, I'm not to bothered about create, but delete, or modify I think is necessary to maintain the database. Others have however asked for create. So, it's not about lying, but error recovery (at least for me).

 
Cheers

Chris

 

@kaikreuzer
Copy link
Member

Ok, so your use case is indeed not "regular use", but really rather db administration/maintenance... That's another sign for me that it does not really belong into the same category (and thus should not necessarily extend the existing interfaces, but should rather be something orthogonal, i.e. an interface in its own right). If we add permissioning features one day, it would also mean that you would only want to grant administrators access to these methods, not the normal "runtime user".
So in some way you are implementing "yet another sql db frontend" - with the only benefit that you can nicely integrate it in HABmin. Not sure if this is really worth the effort - is it so complicated to use some external tools to do database clean ups?

@cdjackson
Copy link
Contributor Author

It's not "too difficult" to use other tools - I have db admin tools, so I can use this for sure. However, in my opinion, it's easier not to implement the interface in HABmin, and this then works for any persistence service that supports read/write, and anyone can use it.

This integrates administration into the system, rather than requiring the user to pull together different tools and dig around in the depths of a database. For me, I can do it, but if we don't have adminstration features, it requires anyone wanting to "fix" the database needs to be technically competent, and know the schema.

I don't really see the problem with extending the existing interfaces. The current interface supports read and write of the persistence database - all we want to do is to add update and delete. It's still standard interfaces into a store?

@cdjackson
Copy link
Contributor Author

Hi @kaikreuzer. In order to progress (and get the mysql binding released so we can get the postgres binding produced), I think I'll revert to using the queryable class for mysql. I do think that we need to put some serious thought into how to manage this sort of thing though since I think management features should be designed in to openHAB, and not to simply require users to sift around in the depths of a database. In this case, the database uses linked tables, and if you mess with this, you will could screw up the store, and not everyone is happy to be typing SQL queries into a database management tool :)

I'm not sure about your thoughts for handling user permissions, but I agree that this possibly is something that could be restricted. I was assuming that this would be restricted at the REST interface rather than within the interface to the persistence store, but I'm not sure about your plans here. Maybe therefore we should define a new interface that can be implemented by stores supporting these features (SQL in general, also DB4O I think).

I don't really think the interface would look at lot different to the CRUD class I've defined, but if this helps separate the functionality, then that's a good reason to do it - I'm open to suggestions. As above though, the one thing I'd really strongly recommend against is not supporting maintenance functions within the system since it limits the use of openHAB to "reasonably techy" users rather than the masses, and that would be a shame since it's such a good system.

Anyway, I look forward to your further thoughts - I know there's a lot of pressures on your time so I appreciate the input.

Cheers
Chris

@ghost ghost assigned kaikreuzer Oct 16, 2013
@cdjackson
Copy link
Contributor Author

Hi @kaikreuzer. One other point regarding the security issue that I should have mentioned, and related to my initial thought that security would be at the REST interface level. With my work with the REST/HABmin interfaces, I have 2 interfaces for persistence - one is through the /rest/config/persistence, and the other is (currently) /rest/persistence. The config/persistence interface provides the delete/update functions, and the persistence interface only provides access to the data and lists of items being persisted (basically to support data analyses functions). Again, this separates the two from a security perspective.

Obviously integration of HABmin REST interfaces is another issue and I don't want to discuss it here, but I just thought I'd add this comment as it should reduce your concern on security?

I look forward to your thoughts on how we move this forward.

Cheers
Chris

@kaikreuzer
Copy link
Member

Hi Chris,

Thanks for your thoughts - I fully understand your points and I agree that admin functionality will make openHAB much better usable for non-tech-savvy users.
My current problem is the right moment: So far, openHAB focuses on "operation" not "administration" features. Introducing administration is a huge step that requires some thoughts and possibly a couple of new concepts. Right now, I want to do the move of the core bundles to the Eclipse SmartHome project - and thus, I am reluctant to introduce major new features now. Once we have the code at Eclipse, I am much more willing to do evolution of the core towards more administration. This also concerns the whole /config extension to the REST API (which I absolutely welcome and definitely don't want to oppose!)
So for the moment, I would appreciate your understanding and put this request on hold - and come back to it at Eclipse.org :-)

@cdjackson
Copy link
Contributor Author

Hi Kai,
Ok - that's fine. I'll continue this in my own fork for now and we can re-align later. I'll revert the mysql service back to standard Queryable service and try and get this out in the coming days.

Do you have a timeframe for when you think you'll be able to consider these "evolution" issues - I think it's important to give people an idea of where the project is going, along with the timeframe - at the moment, this isn't clear (at least not to me).

Cheers
Chris

@kaikreuzer
Copy link
Member

Thanks a lot!
Regarding timeframe, this is mentioned here: http://www.eclipse.org/proposals/technology.smarthome/
So code split/move should happen this year, first release of the SmartHome core some when in Q1/2014.
Let's see how it works out - as it is all only my spare time, it is difficult to plan...

@kaikreuzer kaikreuzer closed this Oct 22, 2013
teichsta pushed a commit that referenced this pull request Sep 7, 2014
marcelrv pushed a commit to marcelrv/openhab1 that referenced this pull request Jun 9, 2016
added ssl certificate generator to demo package
@cdjackson cdjackson deleted the CRUDPersistence branch August 12, 2016 19:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants