bug in the SAN move validation

[lackovic10]

email from Ben:

The SAN for this move in the web UI is an unambiguous "Qhe7" (since only one queen is on the h file): http://note.io/1uGuUvc (in that image I said it wasn't specific enough, but it is... since you only count the white queens!)

So passing "Qhe7" should have been fine. The error that was logged is this:

http://www.chess.com/echess/game?id=92304950

2014-09-08 14:07:09.000

request_url: http://api.chess.com/v1/games/92304950?_format=json&loginToken=4a64b5e5faa41f8f132a5568dbd5b666&signed=Android3_2-e1dfc0934c835dcff347bdd43bd9a3848b62c04d syslog.channel: local4.err level: ERROR hostname: panno.chess.com request loginToken: ~ signed: Android3_2-~ server SCRIPT_NAME: /index_api.php REQUEST_METHOD: GET REDIRECT_URL: /v1/games/92304950 HTTP_USER_AGENT: Chesscom-Android/3.1.10 (Android/4.4.2; DROID RAZR HD; en_US) SERVER_NAME: api.chess.com APP_STAGE: prod controller: Chess\GameBundle\Controller\Api\GameController::getAction env: prod matched_route: api_game_get is_beta: false message exception 'Exception' with message 'Unable to validate SAN Move: Qh7e7 Game id: 92304950' in /www/sites/www.chess.com/src/Chess/GameBundle/GameManager.php:144 Stack trace:

0 /www/sites/www.chess.com/src/Chess/GameBundle/Api/GameManager.php(446): Chess\GameBundle\GameManager->getPiotrMoves(92304950)

#1 /www/sites/www.chess.com/src/Chess/GameBundle/Controller/Api/GameController.php(2113): Chess\GameBundle\Api\GameManager->getPreparedGame(Object(Game), 15137518, 'small')
#2 [internal function]: Chess\GameBundle\Controller\Api\GameController->getAction('4a64b5e5faa41f8...', '92304950', 'small', Array)
#3 /www/sites/www.chess.com/chess/bootstrap.php.cache(1001): call_user_func_array(Array, Array)
#4 /www/sites/www.chess.com/chess/bootstrap.php.cache(975): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Chess\ChessBundle\Request), 1)
#5 /www/sites/www.chess.com/chess/bootstrap.php.cache(1101): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Chess\ChessBundle\Request), 1, true)
#6 /www/sites/www.chess.com/chess/bootstrap.php.cache(411): Symfony\Component\HttpKernel\DependencyInjection\ContainerAwareHttpKernel->handle(Object(Chess\ChessBundle\Request), 1, true)
#7 /www/sites/www.chess.com/htdocs/index_api.php(15): Symfony\Component\HttpKernel\Kernel->handle(Object(Chess\ChessBundle\Request))
#8 {main}

This came from before the new piotr move deploy, so you may need to look up the line numbers from the previous state: https://github.com/ChessCom/chess/blob/1e541a93e87eccbedb5be105a0a5c8eb389a1e56/src/Chess/GameBundle/GameManager.php#L144 https://github.com/ChessCom/chess/blob/1e541a93e87eccbedb5be105a0a5c8eb389a1e56/src/Chess/GameBundle/Api/GameManager.php#L446 https://github.com/ChessCom/chess/blob/1e541a93e87eccbedb5be105a0a5c8eb389a1e56/src/Chess/GameBundle/Controller/Api/GameController.php#L2113

That lists "Qh7e7" as the SAN move. There are two things wrong with that:

a. It's a valid move that doesn't validate. b. It's more specific than needed, the less-specific is what we display in the UI, but this notation is what we have in the PGN for that game. (Attached.) I think this means that this move notation was generated by the Chess-Game library, but then is considered invalid?

Here's teh current dump of the game:

report(read-only)> select * from game where game_id = 92304950\G *** 1. row *** game_id: 92304950 game_type_id: 1 game_location_id: 3 game_location_type_id: 2 game_status_id: 2 initial_setup: NULL game_start_time: 2014-06-23 14:25:04 description: NULL rated: 1 private: 0 user1_id: 10974668 user2_id: 15137518 base_time1: 2592000 time_increment1: NULL user1_rating: NULL user2_rating: NULL user1_result_id: NULL user2_result_id: NULL name: Let's Play! game_end_time: NULL is_already_notified_on_time: 0 move_text: mCZRlB2Ugv92cMXPfA6SblWOeg3VMD!TCKRKBKTJow5ZlC75AJSJdJ0SJt5XadOGjrZItH80dZ08fdYQHA46CIXWdR6YZ780RZYZIZ?7ZTPHAQUMTN2?vB7BQsHzsuBdgoWZNTZQnvdauBaiBW09TZ90ZI07IX78XR87W570R607pFikoxGyFMyrMVrjDM1TKTj~6W7ZWQbhxEkAvDAQ5?zr?3ZRT1Q6DLrjLS65S0j~0~bdELRY1~dZ8ZYP9zPO30 fen: 1r6/3QQ3/k6P/5KB1/1Q6/6P1/8/7q b - - 5 65 ply_count: 129 user_to_move: 2 last_move_time: 2014-09-08 14:07:03 last_move_time2: 2014-09-07 17:57:54 average_rating: NULL user1_draw_offer: 0 user2_draw_offer: 0 move_end_time: 2014-09-11 14:07:03 is_chat_enabled: 1 tournament_id: NULL rating_sum: 1768 last_date: 2014-09-08 14:07:03 imported_in_explorer: 0 game_time_class: NULL thread_id: NULL team1_cgroup_id: NULL team2_cgroup_id: NULL team_match_id: NULL is_takeback: 0 1 row in set (0.35 sec)

I hope this gives you some routes to check out, and then you can decide where the problems are.

[lackovic10]

The PGN string:

[Event "Let's Play!"] [Site "Chess.com"] [Date "2014.06.23"] [White "fcforrest"] [Black "Tnoff"] [Result "*"] [WhiteElo "940"] [BlackElo "897"] [TimeControl "1 in 3 days"]

1.e4 d6 2.d4 g6 3.Nf3 Bg7 4.Bg5 b6 5.Bc4 Be6 6.Nbd2 a6 7.O-O h6 8.Bf4 Nf6 9.e5 dxe5 10.dxe5 Nd5 11.g3 Nd7 12.Ne4 Qb8 13.Bxd5 Bxd5 14.Qxd5 e6 15.Qd3 Qb7 16.Rad1 a5 17.b3 Nc5 18.Qb5+ Ke7 19.Rd7+ Ke8 20.Rfd1 c6 21.Qc4 Rc8 22.Nxc5 Qa7 23.R1d6 Rc7 24.Rd8+ Ke7 25.R6d7+ Rxd7 26.Nxd7 Rxd8 27.Nf6 b5 28.Qxc6 g5 29.Nh5 Bh8 30.Nd4 Rxd4 31.Qc3 b4 32.Qe3 Rd1+ 33.Kg2 Qd7 34.Nf6 Qc6+ 35.f3 Ra1 36.Qd4 Rxa2 37.Qa7+ Kf8 38.Nd7+ Ke7 39.Nc5+ Kd8 40.Nb7+ Ke8 41.Nd6+ Kd8 42.Qb8+ Ke7 43.Nc8+ Kd8 44.h4 Rxc2+ 45.Kh3 a4 46.hxg5 axb3 47.gxh6 b2 48.Bg5+ f6 49.exf6 b1=Q 50.Na7+ Kd7 51.Nxc6 Qh1+ 52.Kg4 Rc4+ 53.f4 Rxc6 54.Qxh8 b3 55.Qh7+ Kd6 56.f7 Rc8 57.f5 b2 58.fxe6 Rb8 59.e7 b1=Q 60.e8=Q Qbd1+ 61.Kf5 Kc7 62.f8=Q+ Qd7+ 63.Qexd7+ Kb6 64.Qb4+ Ka6 65.Qh7e7 *

[lackovic10]

my email:

i've investigated the problem and the library, i believed i have fixed the problem, but not sure how. i mean i know how i fixed it, but i'm not sure what that code part is exactly doing.

the error is thrown in ChesscomGame::GetCmMoveFromSanMove(), which calls ChessGame::_getSquareFromParsedMove() - https://github.com/ChessCom/Chess-Game/blob/master/ChessGame.php#L557

i've printed some data:

• parsedmove:

array (size=5)
  'takesfrom' => boolean false
  'piece' => string 'Q' (length=1)
  'disambiguate' => string 'h7' (length=2)
  'takes' => string '' (length=0)
  'square' => string 'e7' (length=2)

• the _pieces field (printed only the white pieces since the white queen on h7 moves):

array (size=7)
  'WB1' => string 'g5' (length=2)
  'WQ' => string 'h7' (length=2)
  'WK' => string 'f5' (length=2)
  'WP4' => 
    array (size=2)
      0 => string 'b4' (length=2)
      1 => string 'Q' (length=1)
  'WP6' => 
    array (size=2)
      0 => string 'd7' (length=2)
      1 => string 'Q' (length=1)
  'WP7' => 
    array (size=2)
      0 => string 'g3' (length=2)
      1 => string 'P' (length=1)
  'WP8' => 
    array (size=2)
      0 => string 'h6' (length=2)
      1 => string 'P' (length=1)

Looks to me, that this variable ($this->_pieces) contains each piece that's left on the board, along with it's position. in case of pawns the original piece is displayed in the key, and current or the promoted one (if the pawn has been promoted) in the value which is now an array: (position, piece).

the $square variable has the value h7 in our error case (Qhe7)

i've updated the code here: https://github.com/ChessCom/Chess-Game/blob/master/ChessGame.php#L619-622 to check if $value is an array. if true, check $value[0] == $square, like it is now. if false, check $value == $square

my fix, instead of these lines

if ($name{1} == $parsedmove['piece'] &&
    ( (is_array($value) && $value[0] == $square) || (!is_array($value) && $value == $square) )) {
    return $square;
}

i assume that would need to be updated in the whole foreach, if it's a good fix.

Let me know what do you think about this, and do we have someone with more knowledge of the library, or should we go with this fix, or should i try to study the library more deeply and make sure the fix is good

[marcosdsanchez]

@lackovic10 do you think that you can come up with a reduced example of when the bug occurs? It's difficult to digest all the information here and that would really help us to come up with a solution.

A good example is: "When starting with this FEN: "xxxx" , the move "xx" is allowed by the code but it's illegal"

[marcosdsanchez]

Closing the issue. Please reopen if you can come up with a reduced example of the bug.