godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.46k stars 21.07k forks source link

FileAccess store_line() overwriting files instead of appending to files without seek_end() #98243

Closed aaron-bigadventurecrew closed 5 days ago

aaron-bigadventurecrew commented 6 days ago

Tested versions

Reproducible since 4.3-beta3 up until now 4.4-dev3

System information

Windows 10 Pro - Godot v4.3.stable.steam [77dcf97d8]

Issue description

Per the docs, FileAccess' store_line() method is defined as:

void store_line(line: String) Appends line to the file followed by a line return character (\n), encoding the text as UTF-8.

The use of Appends here would imply that it would just add to a file ending with a trailing \n. However, the use of WRITE, WRITE_READ, and READ_WRITE cause the file to be overwritten completely. The only way to truly append a file with store_line() is to use the seek_end() method first.

The Ask: Either (1) Update the docs to more accurately explain what store_line() does, or (2) Revisit the engine code for store_line() and confirm whether it should actually append to a file.

Docs Link: https://docs.godotengine.org/en/stable/classes/class_fileaccess.html#class-fileaccess-method-store-line

Steps to reproduce

extends Node

func _ready() -> void:
    var file_no_seek: FileAccess
    var file_with_seek: FileAccess

    if !FileAccess.file_exists("user://no-seek.txt"):
        file_no_seek = FileAccess.open("user://no-seek.txt", FileAccess.WRITE_READ)
        print("no-seek.txt created at: " + OS.get_user_data_dir() + "/no-seek.txt")
    else:
        file_no_seek = FileAccess.open("user://no-seek.txt", FileAccess.READ_WRITE)
        print("no-seek.txt opened from: " + OS.get_user_data_dir() + "/no-seek.txt")

    file_no_seek.store_line(Time.get_datetime_string_from_system() + " - Attempt to increment no-seek.txt")
    print("no-seek.txt file contents:\n" + file_no_seek.get_as_text())
    file_no_seek.close()
    print("Successfully closed no-seek.txt")

    if !FileAccess.file_exists("user://with-seek.txt"):
        file_with_seek = FileAccess.open("user://with-seek.txt", FileAccess.WRITE_READ)
        print("with-seek.txt created at: " + OS.get_user_data_dir() + "/with-seek.txt")
    else:
        file_with_seek = FileAccess.open("user://with-seek.txt", FileAccess.READ_WRITE)
        print("with-seek.txt opened from: " + OS.get_user_data_dir() + "/with-seek.txt")

    file_with_seek.seek_end()
    file_with_seek.store_line(Time.get_datetime_string_from_system() + " - Attempt to increment with-seek.txt")
    print("with-seek.txt file contents:\n" + file_with_seek.get_as_text())
    file_with_seek.close()
    print("Successfully closed with-seek.txt")

    return

Note the following print() outputs:

no-seek.txt opened from: C:/Users/$ME/AppData/Roaming/Godot/app_userdata/StoreLineRepro/no-seek.txt no-seek.txt file contents: 2024-10-16T10:15:55 - Attempt to increment no-seek.txt

Successfully closed no-seek.txt with-seek.txt opened from: C:/Users/$ME/AppData/Roaming/Godot/app_userdata/StoreLineRepro/with-seek.txt with-seek.txt file contents: 2024-10-16T10:15:37 - Attempt to increment with-seek.txt 2024-10-16T10:15:46 - Attempt to increment with-seek.txt 2024-10-16T10:15:55 - Attempt to increment with-seek.txt

Successfully closed with-seek.txt

This shows store_line() does end with a trailing \n, but even in READ_WRITE it overwrites the file without the use of seek_end() first.

Minimal reproduction project (MRP)

MRP made in v4.3.stable.steam:

StoreLineRepro.zip

syntaxerror247 commented 5 days ago

@aaron-bigadventurecrew see this PR #98238

aaron-bigadventurecrew commented 5 days ago

@syntaxerror247 That PR encapsulates the need well, this issue can be closed once merged. My intention of raising the issue was to give a simple MRP for the team's review anyways since explaining it can be a bit much. Thank you!

aaron-bigadventurecrew commented 5 days ago

Still pending the live docs page to be updated reflecting the PR Merge:

https://docs.godotengine.org/en/stable/classes/class_fileaccess.html#class-fileaccess-method-store-line

Clear browser cache (Ctrl+Shift+R):

Request URL:
https://docs.godotengine.org/en/stable/classes/class_fileaccess.html
Request Method:
GET
Status Code:
200 OK
Referrer Policy:
strict-origin-when-cross-origin

access-control-allow-methods:
HEAD, OPTIONS, GET
access-control-allow-origin:
*
age:
6402
cache-control:
max-age=1200
cdn-cache-control:
public
cf-cache-status:
DYNAMIC
content-encoding:
zstd
content-type:
text/html; charset=utf-8
date:
Wed, 16 Oct 2024 20:08:51 GMT
last-modified:
Fri, 04 Oct 2024 03:31:29 GMT

Follow-up normal refresh:

Request URL:
https://docs.godotengine.org/en/stable/classes/class_fileaccess.html
Request Method:
GET
Status Code:
304 Not Modified
Referrer Policy:
strict-origin-when-cross-origin

access-control-allow-methods:
HEAD, OPTIONS, GET
access-control-allow-origin:
*
age:
6615
cache-control:
max-age=1200
cdn-cache-control:
public
cf-cache-status:
DYNAMIC
date:
Wed, 16 Oct 2024 20:12:24 GMT
etag:
W/"fefc62705638391dc44bb2e0341f4c50"
last-modified:
Fri, 04 Oct 2024 03:31:29 GMT
bruvzg commented 5 days ago

Still pending the live docs page to be updated reflecting the PR Merge:

https://docs.godotengine.org/en/stable/classes/class_fileaccess.html#class-fileaccess-method-store-line

This is a link to the stable docs, it will only be updated when PR is cherry-picked for 4.3, new changes are applied to the latest docs. But sync class reference GitHub action is set to only run every Saturday, so latest docs won't be updated until then.

aaron-bigadventurecrew commented 5 days ago

@bruvzg Oh ok sounds good then! You can mark this as resolved whenever you'd like, the changes to the wording in the PR make sense to me and explain why my reported behavior happened.