Merge into trunk : fm-us-2695 : Code : Unifield Server

Status:	Rejected
Rejected by:	jftempo on 2018-09-14
Proposed branch:	lp:~fabien-morin/unifield-server/fm-us-2695
Merge into:	lp:unifield-server
Diff against target:	201 lines (+100/-6) 7 files modified bin/addons/base/i18n/fr.po (+7/-0) bin/addons/msf_profile/i18n/fr_MF.po (+7/-0) bin/addons/msf_sync_data_server/__openerp__.py (+2/-0) bin/addons/msf_sync_data_server/security/group.xml (+9/-0) bin/addons/sync_client/sync_client.py (+61/-5) bin/osv/orm.py (+6/-1) doc/openerp-server.conf (+8/-0)
To merge this branch:	bzr merge lp:~fabien-morin/unifield-server/fm-us-2695
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
UniField Reviewer Team		2017-04-11	Pending
Review via email: mp+322338@code.launchpad.net

Revision history for this message

Jeff Allen (jr.allen) wrote on 2017-04-11:

#

Sorry, lots of comments.

Revision history for this message

Jeff Allen (jr.allen) on 2017-04-11:

#

Revision history for this message

Jeff Allen (jr.allen) on 2017-04-11:

#

Revision history for this message

Fabien MORIN (fabien-morin) wrote on 2017-04-12:

#

Download full text (9.8 KiB)

On Tuesday 11 April 2017 14:31:06 you wrote:
> Sorry, lots of comments.
>
> Diff comments:
> > === modified file 'bin/updater.py'
> > --- bin/updater.py 2017-01-17 13:07:11 +0000
> > +++ bin/updater.py 2017-04-11 07:12:45 +0000
> > @@ -445,42 +445,91 @@
> >
> > def reconnect_sync_server():
> > """Reconnect the connection manager to the SYNC_SERVER if password
> > file
> > exists
> >
> > + If password file is not found but sync_user_login and
> > sync_user_password are + found in the config file, set them into the
> > connection manager, this way + there is no need to enter them
> > manually.
> >
> > """
> > import tools
> >
> > + import pooler
> >
> > credential_filepath = os.path.join(tools.config['root_path'],
> > 'unifield-socket.py')>
> > - if os.path.isfile(credential_filepath):
> > - import base64
> > - import pooler
> > - f = open(credential_filepath, 'r')
> > - lines = f.readlines()
> > - f.close()
> > - if lines:
> > - try:
> > + sync_user_login = tools.config.get('sync_user_login')
>
> if sync_user_login == 'admin': throw an error. We only want them to be using
> this feature with the per-instance user/pass.

Throwing an error is may be a bit violent. May be it is better to ignore it an
log an error message than preventing the instance from starting ?

> > + sync_user_password = tools.config.get('sync_user_password')
> > + dbname_list = []
> > + password_from_file = None
> > + cr = None
> > +
> > + try:
> > + if os.path.isfile(credential_filepath):
> > + import base64
> > + with open(credential_filepath, 'r') as f:
> > + lines = f.readlines()
> >
> > + if lines:
> > dbname = base64.decodestring(lines[0])
> >
> > - password = base64.decodestring(lines[1])
> > - logger.info('dbname = %s' % dbname)
> > - db, pool = pooler.get_db_and_pool(dbname)
> > - db, pool = pooler.restart_pool(dbname) # do not remove
> > this line, it is required to restart pool not to have - #
> > strange behaviour with the connection on web interface -
> > - # do not execute this code on server side
> > - if not pool.get("sync.server.entity"):
> > - cr = db.cursor()
> > - # delete the credential file
> > - os.remove(credential_filepath)
> > - # reconnect to SYNC_SERVER
> > - connection_module =
> > pool.get("sync.client.sync_server_connection") -
> > connection_module.connect(cr, 1, password=password) -
> > - # in caes of automatic patching, relaunch the sync
> > - # (as the sync that launch the silent upgrade was
> > aborted to do the upgrade first) - if
> > connection_module.is_automatic_patching_allowed(cr, 1): -
> > pool.get('sync.client.entity').sync_withbackup(cr, 1) -
> > cr.close()
> > - except Exception as e:
> > - ...

On Tuesday 11 April 2017 14:31:06 you wrote:
> Sorry, lots of comments.
> 
> Diff comments:
> > === modified file 'bin/updater.py'
> > --- bin/updater.py	2017-01-17 13:07:11 +0000
> > +++ bin/updater.py	2017-04-11 07:12:45 +0000
> > @@ -445,42 +445,91 @@
> > 
> >  def reconnect_sync_server():
> >      """Reconnect the connection manager to the SYNC_SERVER if password
> >      file
> >      exists
> > 
> > +    If password file is not found but sync_user_login and
> > sync_user_password are +    found in the config file, set them into the
> > connection manager, this way +    there is no need to enter them
> > manually.
> > 
> >      """
> >      import tools
> > 
> > +    import pooler
> > 
> >      credential_filepath = os.path.join(tools.config['root_path'],
> >      'unifield-socket.py')> 
> > -    if os.path.isfile(credential_filepath):
> > -        import base64
> > -        import pooler
> > -        f = open(credential_filepath, 'r')
> > -        lines = f.readlines()
> > -        f.close()
> > -        if lines:
> > -            try:
> > +    sync_user_login = tools.config.get('sync_user_login')
> 
> if sync_user_login == 'admin': throw an error. We only want them to be using
> this feature with the per-instance user/pass.

Throwing an error is may be a bit violent. May be it is better to ignore it an 
log an error message than preventing the instance from starting ?

> > +    sync_user_password = tools.config.get('sync_user_password')
> > +    dbname_list = []
> > +    password_from_file = None
> > +    cr = None
> > +
> > +    try:
> > +        if os.path.isfile(credential_filepath):
> > +            import base64
> > +            with open(credential_filepath, 'r') as f:
> > +                lines = f.readlines()
> > 
> > +            if lines:
> >                  dbname = base64.decodestring(lines[0])
> > 
> > -                password = base64.decodestring(lines[1])
> > -                logger.info('dbname = %s' % dbname)
> > -                db, pool = pooler.get_db_and_pool(dbname)
> > -                db, pool = pooler.restart_pool(dbname) # do not remove
> > this line, it is required to restart pool not to have -                #
> > strange behaviour with the connection on web interface -
> > -                # do not execute this code on server side
> > -                if not pool.get("sync.server.entity"):
> > -                    cr = db.cursor()
> > -                    # delete the credential file
> > -                    os.remove(credential_filepath)
> > -                    # reconnect to SYNC_SERVER
> > -                    connection_module =
> > pool.get("sync.client.sync_server_connection") -                   
> > connection_module.connect(cr, 1, password=password) -
> > -                    # in caes of automatic patching, relaunch the sync
> > -                    # (as the sync that launch the silent upgrade was
> > aborted to do the upgrade first) -                    if
> > connection_module.is_automatic_patching_allowed(cr, 1): -                
> >        pool.get('sync.client.entity').sync_withbackup(cr, 1) -           
> >         cr.close()
> > -            except Exception as e:
> > -                message = "Impossible to automatically re-connect to the
> > SYNC_SERVER using credentials file : %s" -               
> > logger.error(message % (unicode(e)))
> > -
> > +                dbname_list.append(dbname)
> > +                password_from_file = base64.decodestring(lines[1])
> > +
> > +        # in case there is no file for automatic reconnecion after
> > patching, +        # then get the list of all db and try to set the
> > login/password on all +        if not dbname_list and sync_user_login and
> > sync_user_password: +            from service.web_services import db
> > +            # try to set up the login/password on all databases listed
> > +            dbname_list = db().exp_list()
> > +
> > +        # remove database name that looks to be SYNC_SERVER
> 
> This is already handled below with "if pool.get("sync.server.entity")"

It is not the same : "if pool.get("sync.server.entity")" requires to know the 
pool which is long and costly operation, so this check permit to remove most 
of the SYNC_SERVER with a ridiculous cost.

> 
> > +        dbname_list = [x for x in dbname_list if 'SYNC' not in x]
> > +
> > +        for dbname in dbname_list:
> > +            logger.info('reconnect_sync_server: dbname = %s' % dbname)
> > +            db, pool = pooler.get_db_and_pool(dbname)
> > +            db, pool = pooler.restart_pool(dbname)  # do not remove this
> > line, +            # it is required to restart pool not to have strange
> > behaviour +            # with the connection on web interface
> > +
> > +            # do not execute this code on server side
> 
> seems maybe like this should be the first thing in the loop?

I don't understand where do you want to put it ? you mean before the line 
"logger.info('reconnect_sync_server: dbname = %s' % dbname)" ?
Then how do you know the pool if you call pool.get("sync.server.entity") 
before the definition of pool ?

> 
> > +            if pool.get("sync.server.entity"):
> > +                continue

This is a second security as we can imagine a sync server instance whitout 
'SYNC' in its name.

> > +
> > +            cr = db.cursor()
> > +            # delete the credential file
> 
> This belongs up above, right after the file is read, not in this loop.
> 
> > +            if os.path.isfile(credential_filepath):
> > +                os.remove(credential_filepath)
> > +
> > +            connection_module =
> > pool.get("sync.client.sync_server_connection") +            if not
> > connection_module:
> > +                logger.error("Module 'sync.client.sync_server_connection'
> > " +                    "not found on %s, not possible to setup sync
> > credentials" +                    " automatically" % dbname)
> > +                continue
> > +
> > +            if password_from_file:
> > +                logger.info('Automatic reconnection to the SYNC_SERVER')
> > +                # reconnect to SYNC_SERVER
> > +                connection_module.connect(cr, 1,
> > password=password_from_file) +
> > +                # in caes of automatic patching, relaunch the sync
> 
> caes -> cases
> Also: It seems like this will result on a sync on every server start when
> sync_user_password is in the config file. (Because production instances run
> with automatic patching enabled all the time.)

why do you think that ?
If you read the code carefully, you will see there is two options:
1- sync_user_login and sync_user_password are defined in the config file
2- automatic patching is activated and password_from_file is not None (mean we 
are during a restart during patch apply)

if the option 2, then the option 1 is ignored as the option 2 already set up 
the credential AND connect to the SYNC_SERVER

otherwise, if not option 2 but option 1, then credential are set BUT as 
requested in the ticket, the instance is NOT connected to SYNC_SERVER and NO 
sync is launched.

If you understand something else, maybe I have to improve the comments and/or 
code clarity.

> > +                # (as the sync that launch the silent upgrade was aborted
> > to do the upgrade first) +                if
> > connection_module.is_automatic_patching_allowed(cr, 1): +                
> >    pool.get('sync.client.entity').sync_withbackup(cr, 1) +               
> > break
> > +            else:
> > +                connection_ids = connection_module.search(cr, 1, [])
> > +                data_to_write = {
> > +                        'login': sync_user_login,
> > +                        'password': sync_user_password,
> > +                }
> > +                if connection_ids:
> > +                    logger.info('Automatic set up of sync connection
> > credentials') +                    connection_module.write(cr, 1,
> > connection_ids, data_to_write) +                    cr.commit()
> > +    except Exception as e:
> > +        if os.path.isfile(credential_filepath) or password_from_file:
> > +            message = "Impossible to automatically re-connect to the
> > SYNC_SERVER using credentials file: %s" +        else:
> > +            message = "Error durring the automatic setting of
> > synchronization credentials on the sync_server_connection: %s" +       
> > logger.error(message % (unicode(e)))
> > +    finally:
> > +        if cr is not None:
> > +            cr.close()
> > 
> >  def check_mako_xml():
> >      """
> > 
> > === modified file 'doc/openerp-server.conf'
> > --- doc/openerp-server.conf	2017-03-01 14:42:57 +0000
> > +++ doc/openerp-server.conf	2017-04-11 07:12:45 +0000
> > @@ -50,3 +50,9 @@
> > 
> >  # for Weeks (0=Monday). See
> >  #
> >  https://docs.python.org/2/library/logging.handlers.html#timedrotatingfil
> >  ehandler log_user_xmlrpc_when = D
> > 
> > +
> > +# credentials to connect to the SYNC_SERVER
> > +# if define, it will automatically setup this credentials after server
> > start
> Credentials to connect to the sync server. If both are not False, the server
> will automatically enter synchronization state "Connected" on startup. ONLY
> FOR USE WITH per-instance sync username/password.

I think there is some misunderstanding of the ticket from me or from you here:
"So here we have to reuse updater.py reconnect_sync_server, but if the 
user/password are retrieved in the config file we do NOT reconnect the 
instance but only set the password."

From that ticket comment, I understand that this ticket will NOT autoconnect 
the instance to the SYNC_SERVER but just set up the credentials, so it means 
state "Not Connected". Am I wrong ?

> 
> > +# then the user just have to mannually connect
> > +sync_user_login = False
> > +sync_user_password = False

-- 
Fabien MORIN
TeMPO Consulting
20, avenue de la Paix
67000 Strasbourg
France

http://www.tempo-consulting.fr
Tel   : +33 3 88 56 82 16
Fax   : +33 9 70 63 35 46

lp:~fabien-morin/unifield-server/fm-us-2695 updated on 2017-04-25

4286. By Fabien MORIN on 2017-04-12: US-2695 [IMP] make possible for connect() to accept a login in addition to
password.
[IMP] close connection on each pass of the for loop to avoid connection leak
[FIX] misspelling
[IMP] delete the credentials file used for automatic patching only when
operations are done (finally).
[IMP] comments
4287. By Fabien MORIN on 2017-04-12: US-2695 [IMP] revert changes to bin/updater.py after ticket description changes
4288. By Fabien MORIN on 2017-04-12: US-2695 [IMP] change the way it works after ticket desciption change:
Now it's a synchronization time that the config file is read to try to connect
with this credentials.
4289. By Fabien MORIN on 2017-04-12: US-2695 [IMP] update the comment accordingly
4290. By Fabien MORIN on 2017-04-13: US-2695 [ADD] translation
[IMP] comments and variable names more clear
4291. By Fabien MORIN on 2017-04-25: US-2695 [MERGE] with latest trunk

Revision history for this message

jftempo (jfb-tempo-consulting) on 2017-04-25:

#

lp:~fabien-morin/unifield-server/fm-us-2695 updated on 2017-04-26

4292. By Fabien MORIN on 2017-04-25: US-2695 [IMP] the group Instance Sync is only for sync_server, move it into
msf_sync_data_server module
[IMP] do not duplicate AdminLoginException error message, make it part of the
exception to have it only one time.
4293. By Fabien MORIN on 2017-04-26: US-2835 [IMP] add a more verbose message in case the object_id is not found
4294. By Fabien MORIN on 2017-04-26: US-2835 [FIX] no need to do any checks on orm_memory as we are already in the
orm_memory class
4295. By Fabien MORIN on 2017-04-26: US-2695 [ADD] translation for error message

Unifield Server

Merge lp:~fabien-morin/unifield-server/fm-us-2695 into lp:unifield-server

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers

 === modified file 'bin/addons/base/i18n/fr.po'
 --- bin/addons/base/i18n/fr.po	2016-12-15 16:14:13 +0000
 +++ bin/addons/base/i18n/fr.po	2017-04-26 15:01:47 +0000
@@ -9695,3 +9695,10 @@
  #: selection:base.language.install,lang:0
  msgid "Russian / русский язык"
  msgstr "Russie / русский язык"
++
++#. module: base
++#: code:addons/orm.py:2348
++#, python-format
++msgid "Object id \'%s\' not found in \'%s\'. You may try to access a deleted temporary object (ie. wizard)"
++msgstr "L'object d'id \'%s\' n'a pas été trouvé pour \'%s\'. Vous essayez probablement d'accéder à un objet temporaire qui n'existe plus (ex. un assistant)"
++
 === modified file 'bin/addons/msf_profile/i18n/fr_MF.po'
 --- bin/addons/msf_profile/i18n/fr_MF.po	2017-04-19 14:11:12 +0000
 +++ bin/addons/msf_profile/i18n/fr_MF.po	2017-04-26 15:01:47 +0000
@@ -98226,6 +98226,13 @@
  msgstr "L'article doit être uniquement utilisé dans un programme correctement construit, à besoin d'une description plus détaillée."
  #. module: sync_client
++#: code:addons/sync_client/sync_client.py:92
++#: code:addons/sync_client/sync_client.py:249
++#, python-format
++msgid "Not connected to server. You cannot use 'admin' in the config file for automatic connection, please use a user dedicated to the synchronization or manually connect before launching a sync."
++msgstr "Non connecté au serveur. Vous ne pouvez pas utiliser 'admin' dans le fichier de configuration pour une connexion automatique, veuillez utiliser un utilisateur dédié à la synchronisation ou vous connecter manuellement avant de lancer une synchronisation."
++
++#. module: sync_client
  #: view:sync.client.update_received:0
  msgid "Updates Received"
  msgstr "Mises à jour Reçues"
 === modified file 'bin/addons/msf_sync_data_server/__openerp__.py'
 --- bin/addons/msf_sync_data_server/__openerp__.py	2016-05-26 08:28:49 +0000
 +++ bin/addons/msf_sync_data_server/__openerp__.py	2017-04-26 15:01:47 +0000
@@ -33,6 +33,8 @@
          'data/sync.server.group_type.csv',
          'data/sync_server.sync_rule.csv',
          'data/sync_server.message_rule.csv',
++        'security/group.xml',
++        'security/ir.model.access.csv',
      ],
      'demo_xml': [
      ],
 === added directory 'bin/addons/msf_sync_data_server/security'
 === added file 'bin/addons/msf_sync_data_server/security/group.xml'
 --- bin/addons/msf_sync_data_server/security/group.xml	1970-01-01 00:00:00 +0000
 +++ bin/addons/msf_sync_data_server/security/group.xml	2017-04-26 15:01:47 +0000
@@ -0,0 +1,9 @@
++<?xml version="1.0" encoding="utf-8"?>
++<openerp>
++	<data>
++		<record id="instance_sync_user" model="res.groups">
++        	<field name="name">Instance Sync</field>
++    	</record>
++    </data>
++</openerp>
++
 === modified file 'bin/addons/sync_client/sync_client.py'
 --- bin/addons/sync_client/sync_client.py	2017-02-28 13:41:45 +0000
 +++ bin/addons/sync_client/sync_client.py	2017-04-26 15:01:47 +0000
@@ -62,6 +62,18 @@
      pass
++class AdminLoginException(Exception):
++    def __init__(self):
++        self.value = "Not connected to server. "\
++        "You cannot use 'admin' in the config file for "\
++        "automatic connection, please use a user dedicated "\
++        "to the synchronization or manually connect before "\
++        "launching a sync."
++
++    def __str__(self):
++        return repr(self.value)
++
++
  class BackgroundProcess(Thread):
      def __init__(self, cr, uid, method, context=None):
@@ -80,6 +92,13 @@
              # Check if we are not already syncing
              entity.is_syncing(raise_on_syncing=True)
              # Check if connection is up
++            connection_obj = pool.get('sync.client.sync_server_connection')
++            try:
++                connection_obj.get_connection_from_config_file(cr, uid, context=context)
++            except AdminLoginException as e:
++                connected = False
++                raise osv.except_osv(_("Error!"),
++                        _(e.value))
              if not pool.get('sync.client.sync_server_connection').is_connected:
                  connected = False
                  raise osv.except_osv(_("Error!"), _("Not connected: please try to log on in the Connection Manager"))
@@ -222,11 +241,18 @@
                      if need_connection:
                          # Check if connection is up
--                        if not self.pool.get('sync.client.sync_server_connection').is_connected:
++                        connection_obj = self.pool.get('sync.client.sync_server_connection')
++                        if not connection_obj.is_connected:
                              if fn.__name__ == 'sync_manual_withbackup':
                                  self.pool.get('backup.config').exp_dump_for_state(cr, uid, 'beforemanualsync', context=context)
--
--                            raise osv.except_osv(_("Error!"), _("Not connected: please try to log on in the Connection Manager"))
++                            # try to coonect from the file
++                            try:
++                                if not connection_obj.get_connection_from_config_file(cr,
++                                            uid, context=context):
++                                    raise osv.except_osv(_("Error!"), _("Not connected: please try to log on in the Connection Manager"))
++                            except AdminLoginException as e:
++                                raise osv.except_osv(_("Error!"),
++                                        _(e.value))
                          # Check for update (if connection is up)
                          if hasattr(self, 'upgrade'):
                              # TODO: replace the return value of upgrade to a status and raise an error on required update
@@ -1346,7 +1372,35 @@
              raise osv.except_osv('Connection Error','Unknown protocol: %s' % con.protocol)
          return connector
--    def connect(self, cr, uid, ids=None, password=None, context=None):
++    def get_connection_from_config_file(self, cr, uid, ids=None, context=None):
++        '''
++        get credentials from config file if any and try to connect to the sync
++        server with them. Return True if it has been connected using this
++        credentials, False otherwise
++        '''
++        logger = logging.getLogger('sync.client')
++        if not self.is_connected:
++            login = tools.config.get('sync_user_login')
++            if login == 'admin':
++                raise AdminLoginException
++            password = tools.config.get('sync_user_password')
++            if login and password:
++                # write this credentials in the connection manager to be
++                # consistent with the credentials used for the current
++                # connection and what is in the connection manager
++                connection_ids = self.search(cr, 1, [])
++                if connection_ids:
++                    logger.info('Automatic set up of sync connection credentials')
++                    data_to_write = {
++                        'login': login,
++                        'password': password,
++                    }
++                    self.write(cr, 1, connection_ids, data_to_write)
++                    cr.commit()
++                return self.connect(cr, 1, password=password, login=login)
++        return False
++
++    def connect(self, cr, uid, ids=None, password=None, login=None, context=None):
          """
          connect the instance to the SYNC_SERVER instance for synchronization
          """
@@ -1366,7 +1420,9 @@
                      con.password = password
                  else:
                      self._password = con.login
--            cnx = rpc.Connection(connector, con.database, con.login, self._password)
++            if login is None:
++                login=con.login
++            cnx = rpc.Connection(connector, con.database, login, self._password)
              con._cache = {}
              if cnx.user_id:
                  self._uid = cnx.user_id
 === modified file 'bin/osv/orm.py'
 --- bin/osv/orm.py	2017-04-20 09:59:19 +0000
 +++ bin/osv/orm.py	2017-04-26 15:01:47 +0000
@@ -2342,7 +2342,12 @@
                  upd_todo.append(field)
          for object_id in ids:
              self._check_access(user, object_id, mode='write')
--            self.datas[object_id].update(vals2)
++            try:
++                self.datas[object_id].update(vals2)
++            except KeyError:
++                error_message = _('Object id \'%s\' not found in \'%s\'. You may try to access a deleted temporary object (ie. wizard)')
++                error_message = error_message % (object_id, self._name)
++                raise except_orm(_('Error'), error_message)
              self.datas[object_id]['internal.date_access'] = time.time()
              for field in upd_todo:
                  self._columns[field].set_memory(cr, self, object_id, field, vals[field], user, context)
 === modified file 'doc/openerp-server.conf'
 --- doc/openerp-server.conf	2017-03-01 14:42:57 +0000
 +++ doc/openerp-server.conf	2017-04-26 15:01:47 +0000
@@ -50,3 +50,11 @@
  # for Weeks (0=Monday). See
  # https://docs.python.org/2/library/logging.handlers.html#timedrotatingfilehandler
  log_user_xmlrpc_when = D
++
++# Credentials to connect to the sync server. At synchronization time,
++# If both are not False, the server will automatically try to connect
++# with this credential and enter synchronization state "Connected"
++# ONLY FOR USE WITH per-instance sync username/password ('admin' login is
++# forbidden)
++sync_user_login = False
++sync_user_password = False