HomePhabricator

Update patch set 8
1801d93c43c1Unpublished

Authored by thiemowmde on Oct 24 2017, 8:45 AM.

Unpublished Commit · Learn More

Not On Permanent Ref: This commit is not an ancestor of any permanent ref.
This commit no longer exists in the repository. It may have been part of a branch which was deleted.This commit has been deleted in the repository: it is no longer reachable from any branch, tag, or ref.

Description

Update patch set 8

Patch Set 8:

I'm note sure what I can add what I haven't already said. Also note that I already moved this patch to an other, future ticket: T166691.

I cannot really see that it will/should be used in some API code […]

How are you going to deserialize the Lexeme serializations users are providing via the wbeditentity API then? You would need to duplicate the deserializer then, and the only difference will be the check we are discussing here.

What is the point of such a duplication? You talk about safety. Sure. I'm not opposing this. But this safety check is done on the wrong level. It can not be the job of a deserializer to check if the database is consistent or not. How could it? This class can not know. It does not have access to the database. Even if you make the nextFormId field non-optional, it can still contain a number that must be considered invalid later when trying to store this Lexeme to the database.

Example:

  • Lexeme L1 contains two Forms, F1 and F2. nextFormId is 3.
  • F2 is deleted. nextFormId is still 3.
  • The LexemeDeserializer gets an invalid serialization for L1 with F1, and nextFormId set to 2.
  • How can the LexemeDeserializer know this is invalid?

In this scenario the provided nextFormId is worthless. It's not better than just guessing the nextFormId based on the maximum Form ID.

So what's the point of making the field non-optional, if this restriction doesn't make anything better?

I used the term "brain-dead". The only way to make this Deserializer not "brain-dead" any more is to connect it to the database, which I hope we agree is the wrong approach. This must be done further down in the actual storage layer. The deserializer infrastructure is not part of that.

Patch-set: 8

Details

Committed
Gerrit Code Review <gerrit@wikimedia.org>Oct 24 2017, 8:45 AM
ChangeId
None

Event Timeline

Gerrit Code Review <gerrit@wikimedia.org> committed rEWLE1801d93c43c1: Update patch set 8 (authored by thiemowmde).Oct 24 2017, 8:45 AM

Empty Commit

This commit is empty and does not affect any paths.