Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update local properties when updating spreadsheet/worksheet #881

Closed
lavigne958 opened this issue May 24, 2021 · 11 comments
Closed

Update local properties when updating spreadsheet/worksheet #881

lavigne958 opened this issue May 24, 2021 · 11 comments
Assignees

Comments

@lavigne958
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
Updating spreadsheet property call does not update local properties.

ex: call to append_row does not increase property row_count

Describe the solution you'd like
Update local properties when necessary.

Describe alternatives you've considered
One can make the explicit call to sht.fetch_sheet_metadata() to update the properties with latest known values from the API.

Additional context
Warning this only solves the issue for SpreadSheets accessed via a single script. If an other script or a human using a web browser accesses the SpreadSheet and updates it, then the values known localy are still invalid.

If you are in that case then you must call sht.fetch_sheet_metadata() to prevent concurent accesses etc.

relates to #545

@lavigne958
Copy link
Collaborator Author

lavigne958 commented Oct 4, 2021

Issue

This issue raises the following problems:

  • update the spreadsheet (and underlying worksheet) is not reflected locally
  • not having the latests state of the spreadsheet locally brings some unexpected behavior
    • updating the row or column count brings column/row insertion to the wrong place
    • updating the title can break any function that tries to get a value using a range (because ranges are build using sheet title)
  • the spreadsheet can be updated online using the UI and the running script can't be notified about the changes.

Requirements

To solve we must ensure the following requirements:

  • The use of gspread can be limited by the number of API calls, this should not be raised extensively otherwise users will be blocked and won't be able to send any more requests for some time.
  • The local spreadsheet properties should reflect what has been update online.

Possible solutions

update local properties after successful update call

  • for any method that updates the spreadsheet properties:

  • update the local properties

  • solves the basics cases and bug pointed by issues linked to this issue.

  • does not solves concurrent access to the spreadsheet (multiple scripts accessing the same spreadsheet)

  • after an update, getting the same spreadsheet can result in different properties (so different behaviors) if spreadhseet has been updated in between

fetch latest spreadsheet properties after every update call

  • for any method that updates the spreadsheet properties

    • fetch the spreadsheet properties
  • solves the basics cases and cases when someone else update the spreadsheet online (bypasses the running script).

  • increases the number of API calls by a lot.

  • This is not suitable for production for heavy usage of google API

Proposal

Update local properties and provide convenient method to fetch properties

  • When a method updates a properties online, on successful call update local property too
    • append a new column will increase the column count, etc...
  • Provide a new method that will fetch the latest spreadsheet properties and sheet properties.
    • can be called by the users or the library itself at any time, this way the user is in control of the extra API call the library makes.
  • this solution does not extra API calls, it update the local properties based on what the script is sending to the google API and offers a way to fetch the properties.

@lavigne958
Copy link
Collaborator Author

Possible solutions

Use some caching mechanism

  • In some scenarios the user knows it is the only one accessing the spreadsheet

    • no concurrent accesses
  • Some caching mechanism can be activated at the constructor of the spreadsheet

  • it will fetch most of the spreadsheet properties at init

  • it can (only ?) mostly rely on every interactions it makes and assume it is the only one editing the resource

  • In case the first assumption fails the whole thing brakes, changes will be made with possible out of date data

  • This is risky and could break other use of gspread, if the cache is not activated and a get_* method does not make the HTTP call it becomes a critical issue.

Thank to @sandl99 for this proposal

@alifeee
Copy link
Collaborator

alifeee commented Jun 8, 2023

"Fixed" by #1211?

Also the same (but more info here) proposal as #1212

(close one of the this or #1212 as duplicate? not sure which)

@lavigne958
Copy link
Collaborator Author

I agree that #1211 fixes most of them. Did we go over the whole properties we set ? if so yes we can close this issue then.

I think #1212 will be too costly for users.

We can add a new feature for the user to choose either:

  • get properties from API each time it requests a property
  • get the local known property to reduce API calls.

@alifeee
Copy link
Collaborator

alifeee commented Jun 14, 2023

See #1211 (comment) for every property that is set.

@alifeee
Copy link
Collaborator

alifeee commented Jun 14, 2023

calls to self.spreadsheet.values_append do not update the properties row_count and col_count, which is the original issue in this issue.

Perhaps there are other methods which change @propertys of gspread.Worksheet? I am not sure.

Here, we would have to update properties row_count and col_count on calls to append_rows, insert_rows, insert_cols, delete_dimension, and perhaps other methods. Since the solution here seems to be "try remember to update the local properties", I am not sure of what a robust solution would be.

@alifeee
Copy link
Collaborator

alifeee commented Jun 14, 2023

perhaps we could have some kind of wrapper around return self.spreadsheet.batch_update(body) so that the developer is 'forced'/coerced into updating local state when updating props

i.e., rework this signature

gspread/gspread/worksheet.py

Lines 2790 to 2792 in c9c88a7

res = self.spreadsheet.batch_update(body)
self._properties["hidden"] = hidden
return res

@lavigne958
Copy link
Collaborator Author

I thought about it too but too complexe I think 🤔 (the wrapper around batch_update)

I check the code today and when we use append_row we use the internal method resize that increase the row count right ?

@alifeee
Copy link
Collaborator

alifeee commented Jun 14, 2023

gspread.Worksheet.add_rows uses gspread.Worksheet.resize, which does update Worksheet.row_count.

However,

gspread.Worksheet.append_rows uses gspread.Spreadsheet.values_append, which doesn't update Worksheet.row_count.

I am not very familiar with the spreadsheet object, so I am not sure why the implementation differs in this way.

@lavigne958
Copy link
Collaborator Author

this is because they don't use the same API calls.

I see so we miss some of them 🤔

We are still better than before but not yet complete.

We could simply increase the value in the method append_rows ?? that should work right ?

@alifeee
Copy link
Collaborator

alifeee commented Jun 29, 2023

this is now fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants