Better check for whether or not a table is_chrono?

[pnomolos]

The previous check didn't work properly because data_source_sql (which data_source_exists? uses internally) only qualifies the table_name to the current schema if a fully qualified name isn't passed.

This could be made even better if the schema was stripped off table before it's passed in, as
right now this won't work if you pass a fully qualified name in to change_table and also pass temporal: true

Ref: https://apidock.com/rails/v5.1.7/ActiveRecord/ConnectionAdapters/PostgreSQL/SchemaStatements/data_source_sql and https://apidock.com/rails/v5.1.7/ActiveRecord/ConnectionAdapters/PostgreSQL/SchemaStatements/quoted_scope

[pnomolos]

Looks like this project has begun to be maintained again, so I've merged upstream master. If this PR is still useful would it be possible to get it in the next release?

[tagliala]

Hi, any chance to rebase instead of merge and provide a failing test case?

[pnomolos]

@tagliala Yes, I should be able to do that. Most likely won't be until next week, though.

[pnomolos]

@tagliala This is ready for review :)

[tagliala]

Hi, I can suggest to run GitHub actions on your fork to fix specs on older ruby and rails versions

NoMethodError:
       undefined method `uuid' for Random:Class

[tagliala]

I would suggest this change for the moment, if you think it is the same thing (specs are green)

Suggested change

	base_table_name = table.to_s.split(".").last
	data_source_exists?("#{TEMPORAL_SCHEMA}.#{quote_table_name(base_table_name)}") &&
	data_source_exists?("#{HISTORY_SCHEMA}.#{quote_table_name(base_table_name)}")
	table_name_without_schema = quote_table_name(table.to_s.split('.').last)
	on_temporal_schema { data_source_exists?(table_name_without_schema) } &&
	on_history_schema { data_source_exists?(table_name_without_schema) }

[tagliala]

Thanks for the failing spec, but to speed up things I need to better understand what it is the use case that this change is addressing.

Any chance to fork https://github.com/diowa/ruby3-rails7-bootstrap-heroku/tree/chronomodel and add a commit with the minimum amount of code needed to understand the impact of this issue on an existing application?

[pnomolos]

@tagliala I added a spec that did a great job of highlighting where things currently aren't working (having a "." within the table name) and fixed all the issues I could find around it. I tested locally against AR 5/5.2/6/7 and everything appears to be working fine on those AR versions. I'll push a fix for Random.uuid not being available in older versions of Ruby.

[pnomolos]

Hi @tagliala following up about this to see where it's at. Thank you 🙏 !

[pnomolos]

Hi @tagliala Following up about this (and #97), thank you!

[pavlitsky]

Any plans on this and #97 get merged? Thank you!

[tagliala]

Can we have this as a method?

Also with a couple of constants like FUNCTION_PREFIX = 'chronomodel' and FUNCTION_SEPARATOR = '_'

This method should work like function_name(table, 'insert') and return chronomodel_table_insert

[tagliala]

Hi, thanks for your interest in ChronoModel

I can't guarantee that this will be merged. We don't want to introduce new issues in this component and all we are fine with this limitation. Also, I'm concerned about new issues that this can introduce by allowing a different schema path

As per the PR itself, I've left a comment. I'm also interested in a better management of quoted chars in the table name, I would like to understand if it is possible to avoid checks like table_name[0] != '"' and

        def quote_identifier_name(prefix: "", table: "", suffix: "")
          if table[0] == '"' && table[-1] == '"'
            "\"#{prefix}#{table[1..-2]}#{suffix}\""
          else
            "#{prefix}#{table}#{suffix}"
          end