Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#3373 closed defect (fixed)

Python cc data merge function does not properly merge map sub elements

Reported by: tmark Owned by: tmark
Priority: high Milestone: Kea0.9
Component: ~dhcp-ddns(obsolete) Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0.5
Total Hours: 4 Internal?: no

Description

Parallel to the C++ merge function fixed in #3339, there is a merge function in the Python configuration library data module. It needs to recursively merge map sub elements. Currently it does flat replacements.

Subtickets

Change History (10)

comment:1 Changed 6 years ago by tmark

  • Owner set to tmark
  • Status changed from new to assigned

comment:2 Changed 6 years ago by tmark

  • Add Hours to Ticket changed from 0 to 3
  • Total Hours 0 deleted

Modified the function to correctly do deep merges of map elements. Added unit test for specific case that failed when merge used dict.update.

ChangeLog? entry:

7xx.    [bug]       tmark
    Python configuration library now properly merges changes into
    configuration items that are maps of items.  This corrects a 
    defect in which a change to an item in a map of items could 
    committed, only to be lost upon committing a subsequent change
    to same map of items during the same bindctl session.

comment:3 Changed 6 years ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from assigned to reviewing

comment:4 Changed 6 years ago by tmark

  • Priority changed from medium to high

comment:5 Changed 6 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:6 follow-up: Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 3 to 0.5
  • Owner changed from marcin to tmark
  • Total Hours set to 3.5

Reviewed bf76c3bd4855432b96d05b67774c8d53702fe275

Overall the changes are good. Just a few nits.

The merge code could be simplified a bit:

    for key in new.keys():
        if ((key in orig) and (type(orig[key]) == dict)):
            merge(orig[key], new[key])
        else:
            orig[key] = new[key]

You can, but you don't have to do this change, as it doesn't really change the behavior of the code.

Copyright dates should be updated.

Please remember to include ticket number and commit hash in the ChangeLog entry.

I don't need to see this ticket again. Once you're done, please merge.

comment:7 in reply to: ↑ 6 Changed 6 years ago by tmark

Replying to marcin:

Reviewed bf76c3bd4855432b96d05b67774c8d53702fe275

Overall the changes are good. Just a few nits.

The merge code could be simplified a bit:

    for key in new.keys():
        if ((key in orig) and (type(orig[key]) == dict)):
            merge(orig[key], new[key])
        else:
            orig[key] = new[key]

You can, but you don't have to do this change, as it doesn't really change the behavior of the code.

You're quite right. I have to admit I was rather aggravated with this all lib at the time.
I have simplified it.

I also improved the function commentary, which as you pointed out in jabber was grammatically a mess, even for Polglish ;)

Copyright dates should be updated.

Got it

Please remember to include ticket number and commit hash in the ChangeLog entry.

I always do.

I don't need to see this ticket again. Once you're done, please merge.

comment:8 Changed 6 years ago by tmark

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 3.5 to 4

comment:9 Changed 6 years ago by tmark

Changes merged with git da3b0d4f364d069ffdb47723545798ac589fae42
Added ChangeLog entry 768.

Last edited 6 years ago by tmark (previous) (diff)

comment:10 Changed 5 years ago by hschempf

  • Milestone changed from Kea1.0 to Kea0.9
  • Version set to git
Note: See TracTickets for help on using tickets.