[Django] Wert im Queryset wird dubliziert und nimmt Platz von anderem Wert ein

[Vinter]

Hallo,

ich bin gerade einem merkwürdigen Phänomen auf der Spur, für das ich die Hilfe von ein paar Experten benötigte.

Ich habe mir mittels Django ein kleines Hobbyprojekt gebastelt: Ein webbasiertes Tool, dass als Frontend für eine Videospieldatenbank funktioniert und das mir den Inhalt der Datenbank als BBCode ausgibt. Um sich das besser vorzustellen: Dieser (laufend aktualisierte) Forenpost ist das Ergebnis/Ziel meines Tools. Ich gebe die nötigen Daten in das Frontend ein und der Post wird halbautomatisch aktualisiert, ohne, dass ich von Hand Tabellen in BBCode anlegen muss. Das ganze Dingen steht mir online bei Heroku zur Verfügung, mit einer Postgres-DB (entwickelt habe ich auf MySQL).

Heute ist mir allerdings ein Bug in der Ausgabe aufgefallen:



Wie man hier sieht, wird in diesem einen Fall einer der Trailer doppelt angezeigt. Nicht nur der Name des Links ist gleich, auch die hinterlegte URL bei YouTube ist die selbe.
Also habe ich versucht, dem ganzen auf die Spur zu kommen und habe mir erstmal die Datenbank direkt bei Heroku angeschaut. Hier ist der betreffende Ausschnitt:


Wie man hier sieht, ist der "Detective Gameplay"-Trailer keineswegs doppelt in der Datenbank angelegt (was meine erste Vermutung war), stattdessen fehlt in der Ausgabe der "Death May Die"-Trailer.

Also habe ich mich via Shell direkt auf Heroku eingewählt und bin mal meinem Code gefolgt, um zu kontrollieren, was da eigentlich intern genau passiert. Und jetzt wird es interessant.



Ich hole mir zu dem betreffenden Spiel die UUID und anhand der UUID die zugehörigen Trailer. Die Ausgabe entspricht exakt der Ausgabe, die mir Heroku weiter oben gegeben hat. Alle fünf Trailer sind vorhanden und mit der korrekten Bezeichnung.

Aber wenn ich die Values des Querysets durchgehe, so, wie ich es auch in meiner App tue:



Plötzlich taucht der "Detective Gameplay"-Trailer zwei mal auf, der "Death May Die"-Trailer ist verschwunden.

Und ich hab nicht mal eine Idee, an welcher Stelle ich mit der Suche nach dem Bug beginnen soll. Ich weiß weder, wie ich den Bug reproduzieren kann, noch, warum er ausgerechnet bei diesem einen Spiel auftaucht, noch, was ihn verursacht. Hat irgendjemand einen Verdacht, was da passiert?

Danke für eure Hilfe.

[__blackjack__]

Gehst Du das in Deiner App tatsächlich genau so unsinnig durch, also *immer wieder* eine Liste mit 5 Werten erzeugen um dann nur *einen* davon über einen festen *Index* auszuwählen? Ich würde ja jetzt einfach mal vermuten das Du Dich dabei nicht auf die Reihenfolge verlassen kannst, also das `values()` die 5 Elemente nicht jedes mal in der gleichen Reihenfolge liefert. Du hast da zwar ein `order_by()`, aber nach dem Kriterium dort sind vier von den fünf Datensätzen gleich.

[Vinter]

Natürlich nicht, in der App ist das eine Schleife mit Counter

Code: Alles auswählen

for j in range (trailer.count()):

                    if delta == True:
                        if trailer.values()[j]['DeltaYesNo'] == False:
                            all_trailer = (all_trailer +
                            '[url=' + trailer.values()[j]['trailer_url'] + ']'
                            + trailer.values()[j]['trailer_name'] +
                            '[/url]\n'
                            )

                        elif trailer.values()[j]['DeltaYesNo'] == True:
                            all_trailer = (all_trailer +
                            '[url=' + trailer.values()[j]['trailer_url'] + '][color=#FF0000]NEU: [/color]'
                            + trailer.values()[j]['trailer_name'] +
                            '[/url]\n'
                            )
                    elif delta == False:
                        all_trailer = (all_trailer +
                        '[url=' + trailer.values()[j]['trailer_url'] + ']'
                        + trailer.values()[j]['trailer_name'] +
                        '[/url]\n'
                        )
                all_trailer = all_trailer + '\n

[Sirius3]

@Vinter: dann zeig doch den Code, den Du tatsächlich benutzt, und zwar als Text und nicht als Bild.

[Vinter]

Das habe ich gemacht, weil ich insbesondere die Werte im Queryset interessant fand. Hier aber mal der Code meiner Ausgabefunktion:

Code: Alles auswählen

def BBCode(delta):

    bbcode = ''


    #Init BBCode
    bbcode = (

        '[tblo][tbody][tr][th=7]Angekündigte Spiele[/th][/tr]\n'+
        '[tr][th=7]----------------------------------------------------------------------------------------------------------------------------------------------------------------[/th][/tr]\n'+
        '[tr]\n'+
        '[table=0][align=left]Keyart[/align][/table]\n'+
        '[table=0][align=left][/align][/table]\n'+
        '[table=0][align=left]Titel/Trailer[/align][/table]\n'+
        '[table=0][align=left][align=left] [/align][/table]\n'+
        '[table=0][align=left]Release[/align][/table]\n'+
        '[table=0][align=left][align=left] [/align][/table]\n'+
        '[table=0][align=left]Plattformen[/align][/table]\n'+
        '[/tr]\n'+
        '[tr][th=7]----------------------------------------------------------------------------------------------------------------------------------------------------------------[/th][/tr]\n'+
        '[tr]\n'


        )

    current_event = Event.objects.filter(current_event="True")

    if current_event:

        # If delta==false, output contains ALL games.
        # If delta == true, output only contains games with new trailers, which haven't been posted before.
        if delta==False:
            games = Game.objects.filter(events = current_event.values()[0]['event_id']).order_by('game_name')
        elif delta==True:
            games = Game.objects.filter(events = current_event.values()[0]['event_id']).filter(DeltaYesNo = True).order_by('game_name')

        if games:
            
            i = games.count()

            for i in range(i):

                #For current game, get basic information
                game_name = games.values()[i]['game_name']
                game_release = games.values()[i]['release_date']
                game_uuid = games.values()[i]['game_id']

                #For current game, get Keyart
                keyart = Keyart.objects.filter(game_id_id = game_uuid)
                if keyart:
                    keyart_url = keyart.values()[0]['keyart_url']
                    url_split = keyart_url.rsplit('.',1)
                    keyart_url = url_split[0] + 'l.' + url_split[1]

                # For current game, look up all platforms
                # (Testing: To easily swap between platforms in two or three
                # columns, platform output is done external)
                current_game = Game.objects.get(game_name = game_name)
                all_platforms = platformsinthree(current_game)

                # For current game, look up all trailers
                trailer = Trailer.objects.filter(game_id = game_uuid).order_by('-DeltaYesNo','-trailer_date')

                # Write down all trailers for current game
                all_trailer = game_name + '\n'
                for j in range (trailer.count()):

                    # In delta-mode, new trailers will get red marker and
                    # are the first in list
                    if delta == True:
                        if trailer.values()[j]['DeltaYesNo'] == False:
                            all_trailer = (all_trailer +
                            '[url=' + trailer.values()[j]['trailer_url'] + ']'
                            + trailer.values()[j]['trailer_name'] +
                            '[/url]\n'
                            )

                        elif trailer.values()[j]['DeltaYesNo'] == True:
                            all_trailer = (all_trailer +
                            '[url=' + trailer.values()[j]['trailer_url'] + '][color=#FF0000]NEU: [/color]'
                            + trailer.values()[j]['trailer_name'] +
                            '[/url]\n'
                            )

                    # If not in delta-mode, just list all trailers in
                    # in no special order
                    elif delta == False:
                        all_trailer = (all_trailer +
                        '[url=' + trailer.values()[j]['trailer_url'] + ']'
                        + trailer.values()[j]['trailer_name'] +
                        '[/url]\n'
                        )
                all_trailer = all_trailer + '\n                       '

                # Write down one game
                bbcode = ( bbcode +
                    '[tr]\n'+
                    '[table=0][align=left][img]' + keyart_url + '[/img][/align][/table]\n'+
                    '[table=0][align=left][align=left] [/align][/table]\n'+
                    '[table=0][align=left]'+ all_trailer +'[/align][/table]\n'+
                    '[table=0][align=left][align=left] [/align][/table]\n'+
                    '[table=0][align=left]' + game_release + '[/align][/table]\n'+
                    '[table=0][align=left][align=left] [/align][/table]\n'+
                    '[table=0][align=left]' + all_platforms + '[/align][/table][/tr]\n'+
                    '[tr]\n'+
                    '[table=0][align=left] [/align][/table]\n'+
                    '[table=0][align=left] [/align][/table]\n'+
                    '[table=0][align=left] [/align][/table]\n'+
                    '[table=0][align=left] [/align][/table]\n'+
                    '[table=0][align=left] [/align][/table]\n'+
                    '[table=0][align=left] [/align][/table]\n'+
                    '[table=0][align=left] [/align][/table]\n'+
                    '[/tr]\n'

                     )

    # After last game, close BBCode-Brackets
    bbcode = ( bbcode +

        '[/tbody]\n'+
        '[/tblo]\n'

        )

    return bbcode

[__blackjack__]

@Vinter: Aber genau hier machst Du doch diesen Unsinn mit dem iterieren über Index und unglaublich oft Aufrufen von `values()`. Sogar noch schlimmer als im Beispiel vorher. Und das nicht nur um *einen* Wert zu ermitteln, teilweise sogar um mehrere Werte aus einem Datensatz zu ermitteln wird für jeden Wert aus dem Datensatz eine Liste mit allen Datensätzen erstellt, einer heraus gegriffen und ein Wert daraus genommen. Und diesen ganzen Aufwand dann für mehrere Werte immer wieder neu. WTF‽ Selbst wenn das keine Probleme verursachen würde ist der Code doch vollkommen unsinnig kompliziert und aufwändig.

So etwas wie ``for i in range(…):`` wo die Laufvariable dann als Index verwendet wird ist unpythonisch. Man kann direkt über die Ergebnisse iterieren, ohne den unnötigen Umweg über einen Index.

Vergleiche mit literalen `True` und `False` macht man auch nicht. Da kommt ja nur wieder ein `True` oder `False` heraus. Entweder den Wert den man sowieso schon hatte, oder dessen Gegenteil – dafür gibt es dann ``not``.

Bei einem ``if flag == True:`` ist als nächstes ein ``elsif flag == False:`` auch nicht sinnvoll. Was sollte es im zweiten Zweig denn sonst sein? Man braucht hier letztlich aber auch nur einen Zweig, nämlich den wo man die Abfrage durch den einen zusätzlichen Filter erweitert. Denn ansonsten steht in beiden Zweigen ja das gleiche – das braucht und sollte man nicht alles zweimal hinschreiben.

``if games:`` ist überflüssig. Das ist bloss eine zusätzliche Einrückebene, ohne das man da tatsächlich irgend etwas von hat.

Warum verwendest Du überhaupt `values()`?

Bei `keyart_url` haben wir einen Programmierfehler. Wenn `keyart` keine Ergebnisse liefert, dann ist `keyart_url` entweder undefiniert, oder es hat einen Wert von einem vorherigen Schleifendurchlauf. Auch hier ist wieder `values()` komisch/falsch. Man würde hier `first()` erwarten oder wahrscheinlich sogar `get()`. Beziehungsweise das man das ORM auch tatsächlich benutzt und dass das als Fremdschlüsselbeziehung modelliert ist und man das einfach als Attribut von `Game`-Objekt abfragen kann.

Auch für BBCode kann/sollte man ein Template schreiben und das nicht im Code als Zeichenketten zusammenstückeln. Das ist ineffizient und unübersichtlich. Und selbst wenn man das im Code macht, sind da viel zu viele ``+``. Zeichenkettenliterale die nur durch „whitespace“-Zeichen getrennt sind, setzt der Compiler schon beim übersetzen des Quellcodes zu einer Zeichenkette zusammen, das muss man nicht zur Laufzeit machen. Und um Werte in Zeichenketten hinein zu formatieren, gibt es die `format()`-Methode auf Zeichenketten.

Ich sehe da auch keinen Code der BBCode in den Werten die dort in BBCode hineinformatieren escapen würde.

[Sirius3]

Ein erster Zwischenschritt könnte so aussehen:

Code: Alles auswählen

BBCODE_TABLE = (
    '[tblo][tbody][tr][th=7]Angekündigte Spiele[/th][/tr]\n'
    '[tr][th=7]----------------------------------------------------------------------------------------------------------------------------------------------------------------[/th][/tr]\n'
    '[tr]\n'
    '[table=0][align=left]Keyart[/align][/table]\n'
    '[table=0][align=left][/align][/table]\n'
    '[table=0][align=left]Titel/Trailer[/align][/table]\n'
    '[table=0][align=left][align=left] [/align][/table]\n'
    '[table=0][align=left]Release[/align][/table]\n'
    '[table=0][align=left][align=left] [/align][/table]\n'
    '[table=0][align=left]Plattformen[/align][/table]\n'
    '[/tr]\n'
    '[tr][th=7]----------------------------------------------------------------------------------------------------------------------------------------------------------------[/th][/tr]\n'
    '[tr]\n'
    '{}'
    '[/tbody]\n'
    '[/tblo]\n'
)

def generate_bbcode(delta):
    rows = []
    current_event = Event.objects.get(current_event="True")
    if current_event:
        games_query = Game.objects.filter(events = current_event['event_id'])
        if delta:
            games_query = games_query.filter(DeltaYesNo=True)

        for game in games_query.order_by('game_name'):
            game_name = game['game_name']
            game_uuid = game['game_id']

            # TODO: ForeignKey in model
            keyart = Keyart.objects.get(game_id_id=game_uuid)
            if keyart:
                url, suffix = keyart['keyart_url'].rsplit('.', 1)
                keyart_url = "{}1.{}".format(url, suffix)
            else:
                # TODO: Default value
                keyart_url = '???'

            # For current game, look up all platforms
            # (Testing: To easily swap between platforms in two or three
            # columns, platform output is done external)
            current_game = Game.objects.get(game_name=game_name)
            all_platforms = platformsinthree(current_game)

            trailers = Trailer.objects.filter(game_id = game_uuid).order_by('-DeltaYesNo','-trailer_date')
            all_trailer = []
            for trailer in trailers:
                # Write down all trailers for current game
                new = '[color=#FF0000]NEU: [/color]' if delta and trailer['DeltaYesNo'] else ''
                all_trailer.append('[url={}]{}{}[/url]\n'.format(
                    trailer['trailer_url'],
                    new,
                    trailer['trailer_name']
                ))

            # Write down one game
            rows.append([
                '[img]{}[/img]'.format(keyart_url),
                ' ',
                '{}\n{}\n'.format(game_name, all_trailers),
                ' ',
                game['release_date'],
                ' ',
                all_platforms,
            ]
            # empty row
            rows.append([' '] * 7)
    formatted_rows = ('[tr]\n{}[/tr]\n'.format(
        ''.join('[table=0][align=left]{}[/align][/table]\n'.format(cell) for cell in cells)
    ) for row in rows)
    bbcode = BBCODE_TABLE.format(
        ''.join('[tr]\n{}[/tr]\n'.format(row) for row in formatted_rows)
    )
    return bbcode

[Vinter]

Hey Leute!

Danke für die Stilkritik, das werde ich mir ansehen.

Aber das löst ja nicht das Problem, bzw. wenn es das tut, dann nur als Workaround:
Warum steht in dem Queryset einmal der korrekte Wert, aber wenn ich mir die values anschaue, ist der eine Eintrag dubliziert?

[Sirius3]

@Vinter: doch, das löst Dein Problem, denn values() ermittelt immer wieder neu die Einträge, und da die Reihenfolge nicht eindeutig ist, ist Dein Ergebnis auch nicht eindeutig.

[__blackjack__]

@Vinter: Doch das löst das Problem und es ist *kein* Workaround. Also leztlich keine Stilfrage, denn Du hast da ja einen *Fehler* gemacht.

Da ist nichts dupliziert, es ist einfach nur nicht garantiert in welcher Reihenfolge die Datenbank die Datensätze liefert wenn das ORDER BY Kriterium gleich ist. Und wenn Du 5× `values()` abrufst, kann es halt passieren das ein Datensatz mal an zweiter und beim nächsten mal an dritter Stelle ist, und wenn das zufällig dann ist wenn Du den zweiten und dritten Datensatz per Index rauspickst, hast Du halt zwei mal den gleichen Datensatz erwischt.

Das liegt übrigens daran das bei jedem `values()` das `QuerySet` geklont wird und dabei der Cache nicht mitkopiert wird, also *jedes mal* die Datenbankabfrage gemacht wird. Das so zu machen ist keine Stilfrage das ist einfach nur hochgradig unsinnig! Zähl mal die Anzahl der `values()`-Aufrufe und wie oft da unsinniger Weise immer und immer wieder die selben Daten von der DB abgefragt werden.

Und auch die Entscheidung überhaupt `values()` zu verwenden, statt das ORM richtig zu verwenden, halte ich auch nicht für eine Stilfrage. IMHO muss man `values()` begründen können und ich sehe da im vorliegenden Code keinen Grund für. Was glaubst Du hat das hier für einen Vorteil? Warum nicht einfach weglassen?

[DasIch]

UUID als Primary Keys in einer relationellen Datenbank zu benutzen ist äußerst problematisch. Zum einen nehmen sie relativ viel Platz weg zum anderen sind sie zufällig angeordnet und somit verlierst du locality in Datenbank internen Strukturen und dass hat wiederum negative Auswirkungen auf die Performance.

Es gibt UUID ähnliche Varianten die diese Probleme nicht haben aber wesentlich einfacher wäre es einfach keine UUIDs zu nutzen.

Ansonsten würde ich empfehlen mal etwas in Observability zu investieren. Wenn man sowas wie OpenTracing nutzt, ist es nicht zu übersehen wenn man unnötige oder teure Queries macht.

[Vinter]

Da ist nichts dupliziert, es ist einfach nur nicht garantiert in welcher Reihenfolge die Datenbank die Datensätze liefert wenn das ORDER BY Kriterium gleich ist. Und wenn Du 5× `values()` abrufst, kann es halt passieren das ein Datensatz mal an zweiter und beim nächsten mal an dritter Stelle ist, und wenn das zufällig dann ist wenn Du den zweiten und dritten Datensatz per Index rauspickst, hast Du halt zwei mal den gleichen Datensatz erwischt.

Danke, die Antwort verstehe ich.
Allerdings erscheint es mir merkwürdig, dass das Problem immer an exakt der selben Stelle auftaucht. Denn man kann sich den Forenthread ja ansehen (=Ausgabe des Tools): Da sind massenhaft Trailer in der Ausgabe und das Posting wird teilweise mehrmals täglich aktualisiert. Ich würde bei dem beschriebenen Verhalten eigentlich erwarten, dass da ständig die Einträge durcheinander kommen.

Ich werde heute abend mal die Ausgabe in der beschrieben Art und Weise umbauen und schauen, ob sich das Problem dann erledigt hat. Und dann gehe ich im restlichen Programm auf die jagt nach values()

[Vinter]

Ein erster Zwischenschritt könnte so aussehen:

Ich hab gestern mal ein wenig mit deiner Lösung herumprobiert, dabei ist mir jedoch sofort eine Fehlermeldung begegnet.

Auf

Code: Alles auswählen

events = current_event['event_id']

und ähnliche Zuweisungen folgt

TypeError: 'Event' object is not subscriptable

Was in der Shell funktioniert, ist

Code: Alles auswählen

events = current_event.event_id

im Browser selbst erhalte ich allerdings auf die Zeile ein

AttributeError
Exception Value: 'QuerySet' object has no attribute 'event_id'

Und das ist auch der Grund, warum ich irgendwann nach viel fluchen und herumprobieren auf die values() gekommen bin: Das war der einzige Weg, wieder an die Werte zu kommen.

[__blackjack__]

@Vinter: Dann geht der Zeile aber etwas anderes voraus. Du scheinst ”im Browser” `current_event` an ein `QuerySet` zu binden – das hat natürlich kein solches Attribut.

Du benutzt also `values()` weil Du keinen Überblick darüber hast was Deine Objekte für Typen haben. Dabei hilft `values()` aber auch nicht, weil Du ja auch da wissen musst welche Objekte diese Methode haben und welche nicht. Herumprobieren bis es klappt und den Unsinn dann zigfach kopieren ist keine sinnvolle Strategie um programmieren zu lernen.