console - log relevant information in home, user and logs page #2777

Gaetanbrl · 2019-10-16T09:51:05Z

Link to #2267 .

Display logs detail by click to get log relevant information in admin log.

Thank you in advance for review.
@fvanderbiest @landryb @tonio @RemiDesgrange

landryb · 2019-10-17T07:43:49Z

Can we take #2267 (comment) into account ? once the changed field is added to the schema, i think it's hard to migrate its type/size to something else, so might aswell get it large from the start.

Apparently (per https://thoughts-on-java.org/mapping-blobs-and-clobs-with-hibernate-and-jpa/)there are two ways, using the Clob java type instead of String, or just annotate the property and keep using String.

I tried adding @Lob annotion on the changed definition, dropped the changed column in the db with varchar type, it was recreated by hibernate as text type in the DB - modifying an attr on a user works (ie json contains {"new":"editoraaa","field":"description","old":"editor","type":"Attribute was changed for a user"}) but strangely the json is not directly readable in the database field (there's 733070 in the field for that record ?). - and here i will stop poking at this since im no java hell expert.

Gaetanbrl · 2019-10-18T15:11:59Z

Can we take #2267 (comment) into account ?

Unfortunately, we can't take it into account now. I can open a new issue to keep this in mind.

I tried adding @lob annotion on the changed definition

Thank you, hope we can do that soon...

landryb · 2019-11-13T10:37:55Z

Ahem... gentle review poke ? :)

Gaetanbrl · 2019-11-13T10:39:42Z

need to manage conflict / rebase again ...

RemiDesgrange · 2019-11-13T14:26:34Z

Can you make the integration test pass ? Thanks.

Gaetanbrl · 2019-11-13T15:00:25Z

Can you make the integration test pass ? Thanks.

Yes, we can @RemiDesgrange. As said before :

need to manage conflict / rebase

pierrejego · 2019-11-14T08:03:35Z

There are three integration tests on attributes deletion for orgs which failed for the moment. I am trying to find out why.
We can delete those attributes in real GUI, without errors.

pierrejego · 2019-11-15T09:02:57Z

Can we take #2267 (comment) into account ? once the changed field is added to the schema, i think it's hard to migrate its type/size to something else, so might aswell get it large from the start.

Apparently (per https://thoughts-on-java.org/mapping-blobs-and-clobs-with-hibernate-and-jpa/)there are two ways, using the Clob java type instead of String, or just annotate the property and keep using String.

I tried adding @Lob annotion on the changed definition, dropped the changed column in the db with varchar type, it was recreated by hibernate as text type in the DB - modifying an attr on a user works (ie json contains {"new":"editoraaa","field":"description","old":"editor","type":"Attribute was changed for a user"}) but strangely the json is not directly readable in the database field (there's 733070 in the field for that record ?). - and here i will stop poking at this since im no java hell expert.

Last error in integration test was due to attributed changed size. (char varying 255) to small to set all information. If an error occurs when trying to save in database, en Error log is provided in trace, and changed value contains. JSon "error": "Error while inserting admin log in database, see admin log file"

I agree with you @landryb , we should change to Lob, but I am not sure how to do it correctly.

I have added @lob annotation to "changed" attribut from class "AdminLogEntry", this work perfectly for me;
I saw that update is set in webmvc-config.xml so table should be updated at startup , but It could be complicated to reinstall previous version if needed.

Tell me if I need to do something else to take type change to Log into account.

landryb · 2019-11-15T09:05:50Z

I agree with you @landryb , we should change to Lob, but I am not sure how to do it correctly.

I have added @lob annotation to "changed" attribut from class "AdminLogEntry", this work perfectly for me;
I saw that update is set in webmvc-config.xml so table should be updated at startup , but It could be complicated to reinstall previous version if needed.

Tell me if I need to do something else to take type change to Log into account.

I'm no java expert wrt this, but we shouldnt care about 'reinstall previous version if needed', no release was made with changed as varchar; that was only temporary during dev in a separate branch, so there's no compat to keep.

What i found strange was not having the plain text value in the database field....

pierrejego · 2019-11-15T09:59:58Z

I need to make more test before review

pierrejego · 2019-11-15T10:29:49Z

Ok I have done more test.
1 - New instance OK
2 - migration from exisiting instance KO,
hibernate does not change type automaticlly if there is date in it (not on my server), so we will need to Drop previous table, OR make an manual ALTER TABLE Script before migration. Where should I put this type of script from version migration ?

Otherwise it's ready for review @RemiDesgrange :) thanks

landryb · 2019-11-15T15:25:00Z

as discussed over the phone we dont need to care about the varchar->lob migration since the changed field is a new field :)

RemiDesgrange · 2019-11-15T16:06:58Z

The migration should go here (but ping @pmauduit anyway), you should not use Lob to store json in database. Georchestra didn't choose the best opensource database out there to end up usng the DB like if it was MySQL :-) . I am not blaming you for this choice. But using lob (so storing binary) is the not a good idea, because we won't be able to request the JSON from SQL. So every people that build tools around georchestra won't able to just query the data and use it. With JSON type you can. If using JSONB, you can even put indexes on json keys !

Ok solution time: you should pass a JSONObject to AdminLogEntry.

georchestra/console/src/main/java/org/georchestra/console/model/AdminLogEntry.java

Line 48 in f348cc2

private String changed;

This will allow you to remove codes that convert back and forth from String to JSON

fvanderbiest · 2019-11-15T16:12:35Z

The migration should go here

Rather https://github.com/georchestra/georchestra/tree/master/migrations/19.12 (the directory does not exist yet, feel free to create it)

landryb · 2019-11-18T08:31:13Z

Nice, didnt know that json & jsonb were native postgresql data types.

As for the migration, i'm still not sure there should be one, as we're only adding a new field to an existing table, and from my understanding hibernate will create the field if it doesnt exist, so there should be no need for an sql snippet ?

cmangeat

we (@cmangeat & @pmauduit) will discuss more precisely (and conceptually) afterwards, current review comments aim at earning lisibility for further improvments proposals.

Conceptually, here are a summary so far:

Sometimes the logging logic is made onto the controllers, sometimes in the DAO. Our opinion is that it might not be the right place to hook (either in the controller nor in the DAO), this leads to duplicating code in both places. This is probably not the role of the DAO to log, and it should stick with LDAP accesses or so.
To complete the previous point: We were thinking about a "ogc-server-statistics"-like approach with log.info() using the logging framework instead.
The PR replaces the LogDAO by a LogUtils class, and unset the LogDAO from the tests. Integrating a new class is OK, but why not having set a LogUtils in the test classes instead of leaving the field unset ? This leads to unecessary if (logUtils != null) in the actual code.
There is a private field added to the UsersController class which is problematic, as it can lead to race conditions in case of several requests are made simultaneously on the same controller, the controller is supposed to be reentrant. There is another place in the code where such an error is made, we will comment accordingly.
We were wondering if we could avoid the long series of "if" statements to check what and where the objects have been modified, we don't have any clue so far.
Sometimes the log statement is made before the actual modification is made onto the LDAP, sometimes after. We think that it would be more relevant to always log after the modification has been made.
As already reported by @rémiDesgrange, we think that using a more convenient type in the postgresql could simplify queries, as using BLOB type makes it complicated to request against.

cmangeat · 2019-11-19T14:20:56Z

console/src/main/java/org/georchestra/console/ds/AccountDaoImpl.java

-
-    @Autowired
-    private AdminLogDao logDao;
+    private Boolean isPendingChanged;


dead code ?

cmangeat · 2019-11-19T14:21:11Z

console/src/main/java/org/georchestra/console/ds/AccountDaoImpl.java

-    public void setLogDao(AdminLogDao logDao) {
-        this.logDao = logDao;
+    public void setPendingChanged(Boolean isPendingChanged) {
+        this.isPendingChanged = isPendingChanged;


Dead code ?

cmangeat · 2019-11-19T14:21:41Z

console/src/main/java/org/georchestra/console/ds/AccountDaoImpl.java

@@ -217,9 +205,14 @@ public synchronized void update(Account account, Account modified, String origin
        if (hasUserDnChanged(account, modified)) {
            ldapTemplate.rename(buildUserDn(account), buildUserDn(modified));
        }
+        this.setPendingChanged(this.hasUserPendingChanged(account, modified));


Dead code ?

cmangeat · 2019-11-19T14:23:01Z

console/src/main/java/org/georchestra/console/ds/AccountDaoImpl.java

        update(modified, originLogin);
    }

+    public boolean hasUserPendingChanged(Account account, Account modified) {


Dead code ?

cmangeat · 2019-11-19T14:23:59Z

console/src/main/java/org/georchestra/console/ds/RoleDaoImpl.java

-                AdminLogEntry log = new AdminLogEntry(originLogin, user.getUid(), logType, new Date());
-                this.logDao.save(log);
+            // Add log entry when role was added
+            if (originLogin != null && logUtils != null) {


logUtils cant be null ?

cmangeat · 2019-11-19T15:52:54Z