Allow SQLT options to be passed to unique constraints #88

Altreus · 2015-11-03T13:40:03Z

Recreated #87 because I based it off master instead of blead.

This PR does work against the current version of SQLT, but that version of SQLT doesn't actually do anything with the information.

There is a PR on SQLT that will actually cause this to have some effect.

dbsrgits/sql-translator#75

I will try to write some tests for this, but I wanted to get the PR made so I can invite discussion in the meantime.

ribasushi · 2015-11-03T13:55:51Z

Greetings @Altreus

Sigh... this is an unholy shitpile (the API before your changes, not your fault). Currently we have:

add_unique_constraint
add_unique_constraints
name_unique_constraint
unique_constraints
unique_constraint_names
unique_constraint_columns

Adding to this unique_constraints_extra to this clusterfuck is not wise to say the least. Instead please add a ->unique_constraints_info method, make it return a hashref (not a flattened list), with names as keys, and values being a subhashref with columns, extra, etc... fields. Just the way ->columns_info works.

That'd be the starting point, which invalidates what you currently have, so I can't elaborate further. More once the first mockup is up.

Altreus · 2015-11-03T14:06:32Z

I did wonder about creating a single method that returned the whole lot, but I figured that since I'd be adding a new method anyway I should follow suit.

I think you're saying to do what I was originally going to do, so I'll implement that and push the updated branch.

Altreus · 2015-11-03T14:44:41Z

Is this what you were thinking of, @ribasushi ?

ribasushi · 2015-11-03T14:51:46Z

lib/DBIx/Class/ResultSource.pm

@@ -702,7 +702,7 @@ sub sequence {

 =over 4

-=item Arguments: $name?, \@colnames
+=item Arguments: $name?, \@colnames, \%extra?



This is not a useful way to extend the signature - too many unknowns. Make it

$name?, \%uc_info | \@colnames

With both names optional and @colnames marked as discouraged in the docs. I.e. people should be doing:

add_unique_constraints( foo => { ... } bar => { ... } )

just like they do with add_columns today.

This is add_unique_constraint rather than add_unique_constraints. Shall I also change the plural version to accept hashrefs to follow suit?

This makes more sense, actually, now that I see what you're after.

Along these lines yes. Again - look at how add_column(s) does things, model on that.

Altreus · 2015-11-03T15:56:40Z

@ribasushi I've addressed all your points. Having rewritten add_unique_constraint to accept an arrayref or a hashref, I was able to rewrite the docs for add_unique_constraints to be along the lines of "It's add_unique_constraint but more than once".

ribasushi · 2015-11-03T16:19:29Z

lib/DBIx/Class/ResultSource.pm

@@ -731,6 +762,9 @@ only columns in the constraint are searched.
 Throws an error if any of the given column names do not yet exist on
 the result source.

+Note also that the keys C<table>, C<name>, and C<fields> in
+C<sqlt_extra> will be ignored.
+


What does this note actually mean...? Please elaborate

Altreus · 2015-11-03T16:46:03Z

Elaborated on ignored keys, as requested

Switch all spots to a select()-based sleep instead

No notable functional changes, mostly de-File::Spec-ing spots that do not need it ( '/' works on Win32 just fine, and VMS is a looooong way off )

This was a rather long journey (I've been meaning to do this since ~2013). As everything else it turned out more complex than I anticipated. Notably due to having to implement from semi-scratch things that a dev should usually never worry about >:( Just look at the amount of stuff one can't reasonably trust these days (pay attention to the comments): git show 5d54c117 | perl -ne 'print if 155..304' | less git show 358a3aa7 | perl -ne 'print if 407..453' | less There is a tangible difference in the smoke times due to a leaner set of deps (though not as big as one would hope... yet). Sample timings as follows: for n in $(seq 26); do dbic_trv_diffable() { perl -0777 -n -E ' print ( map { "$ENV{cur}: $_\n" } map { split /\r?\n/, $_ } $_ =~ /(^TRAVIS_PERL_VERSION.+)/m, $_ =~ / \QSetting environment variables\E .+? (?:\r?\n)+ (.+?) (?: \r?\n){2} /xms, $_ =~ /(^.*Configuration phase seems to have taken.*)/m, ); say ( $_ =~ /(^[^\n]*?List of loadable modules .+?)^[^\n]*?List of loadable modules/ms ); ' } x=$((112987257 + $n)) &&\ y=$((113113497 + $n)) &&\ echo -e "$x => $y\n========\n" &&\ diff -U0 \ <( wget -qO- s3.amazonaws.com/archive.travis-ci.org/jobs/$x/log.txt | \ cur=$x dbic_trv_diffable )\ <( wget -qO- s3.amazonaws.com/archive.travis-ci.org/jobs/$y/log.txt | \ cur=$y dbic_trv_diffable ) done | less P.S. The above is hideous, yes, but you can run it in your terminal *directly*

ribasushi · 2016-03-03T14:23:38Z

Same here - projecting to look into this in about a week or so. Once again - sorry for the delay. This past month alone has been murder:
$ git diff -w --shortstat 729656c..18a2903
389 files changed, 2278 insertions(+), 1138 deletions(-)

These fixes together with RT#112706 make distbuilding on SighOS possible again Also adjust a number of whitespace checks to work properly, ensuring the distdir built on a normal OS and on Win32 match. Read under diff -w -C

A certain spot in the codebase check whether a relationship is "simple". This additional flag allows to consider coderef conditions as well, instead of simply punting with "not a HASH? - no can do" See next commit for the actual switchover While at it fix a subtle bug introduced in b5ce674 - originally the helper is_literal_value recognized -ident as a valid literal. Later on during the migration into SQLA this logic was lost (I do not exactly recall the details), yet the DBIC side was never adjusted. All callsites were audited to make sure nothing else was missed.

@mzealey

Prompted by a PR from @mzealey, a code audit showed the entire implementation to be severely lacking. Switched to proper relationship resolution, with the added benefit of support for custom conds whenever possible. As of this commit every single relationship introspection now goes through a central point: _resolve_relationship_condition(). No more random ... eq 'HASH' checks all over the place. There should be zero functional changes as a result (aside from better custom cond introspection)

…lver This further simplifies the cognitive surface of the condition resolver API just like 786c1cd and a3ae79e. During the sprint to add at least *some* sanity to the codepath infer_values_based_on was introduced as a stopgap to allow 83a6b24 to somehow proceed forward. Since then the amount of spots where this logic is necessary steadily went down, bringing us to the current place: there is just a single spot in the entire codebase that passes a non-empty inferrence structure. Given the entire codepath is rather baroque, the entire idea of inferrence is pushed to new_related instead, leaving the API of the resolver itself even simpler. There are no known issues as a result, verified by re-running the entire test plan for downstreams as described in 12e7015.

Some external use of DBIx::Class::ParameterizedJoinHack revealed a couple sites where the relationship resolution may unexpectedly, yet non-fatally fail. This protects all the reasonable places (partially reverting b47fb9c), downgrading the exceptions to once-per-callsite warnings. I did not have time to dig to find the underlying problem, there may very well be a real bug lurking around :/ For reproduction of the (now) warnings: see https://github.com/ctrlo/lenio

The encapsulated logic is just too complex to try to replicate externally, especially now that everything within DBIC itself uses this method underneath. Patches to the only widely known user (::Resultset::RecursiveUpdate) will follow shortly

Done as a merge to aid bisecting IFF it ever becomes necessary Wide testing indicates zero need for downstream changes (aside from several warnings)

Introduced for reasons unknown back in 93405cf, it is currently nothing but baggage - especially given the lack of name synchronization as described in one of the comments in 28ef946 grep.cpan.me indicates no use in the wild, so just kill it with fire

A real fix for this ticket is pending, but had to be bumped a bit

@inc

Perl 5.26 will be able to be built with no . in @inc, and Debian are already building their 5.24 without it. To cope with this, do -It/lib -MFoo instead of -Mt::lib::Foo.

Optimize some of the codepaths (do not recurse in spots where it makes no practical difference). Deprecate searches with no explicit select-list ( can't remove it outright due to downstream breakage :/ )

No idea how it never got noticed, but both have been broken since the very first commits that introduced the methods ( 4fa7bc2 / e4bb672 ). While changing them 7 years later is a rather serious modification of behavior, the old way never worked without users having to force-scalar each call site. If someone has been relying on e.g. [ func_rs(...) ] to return actual result objects instead of the resultset instance - things will blow up rather quickly and loudly (aside from the carp()-ed warning encouraging users to switch to scalar ctx explicitly) [ func( ... ) ] of course continues to behave like before (directly returning raw values off the cursor... sigh)

No functional changes (nothing else in lib/ and t/ had to change)

Discouraged legacy sugar, which does not even work properly with multicolumn keys in scalar context. Mark properly as INDIRECT to ensure DBIC does not rely on it anywhere Also adjust the SanityChecker to not complain about shadowing of sugar methods with generated ones (i.e. column accessors) - while unfortunate, this kind of thing happens quite often (especially with such a generic name as 'id') and warning about it would make no sense (left alone that methods which are ..._generated_from_resultsource_metadata generally do not invoke next::method anyway)

Subtly modified in 11e469d, this prevents things like the ::IgnoreWantarray helper from taking effect in this case. An audit of all other wantarray() invoking sites did not reveal other issues

Ensure an upcoming commit will not disturb the established (silly but still) API of the resultset-returning methods. Review, annotate and tighten up spots that have to do with wantarray-like behavior Not using the ASSERT_NO_INTERNAL_WANTARRAY macro as it is about to be retired in a subsequent commit. Instead adjust the INDIRECT guard to correctly interpret eval frames Zero functional changes

It was a good idea for its time, and helped clean up the codebase a lot, but ASSERT_NO_INTERNAL_INDIRECT_CALLS currently covers all its functionality and does so in a way less fragile (stateless) manner Mark several more methods as indirect_sugar, leaving only one forgotten spot for last (see next commit) No functional changes Read under -w

I am not entirely sure how I missed it during 1b822bd, but oh well. This should be the last highly volatile part ( as far as downstream is concerned ). As previously - zero functional changes apart from no longer calling search() at several spots (the SanityChecker ensures none of this results in silent breakage) All spots that *do* require wantarray()-specific behavior remained explicit wrappers for search(), instead of doing the wantarray() check themselves: this is a deliberate choice to allow DBIC::Helpers::ResultSet::IgnoreWantarray or similar libraries to continue operating by simply hooking the search() method

Zero functional changes

This set of commits (again - merge for easier bisect) is exclusively dealing with various wantarray()-aware methods, most notably ::ResultSet::search() Wide smoke of downstream adds only 3 extra dists to the list of "passes tests but warns about indirect-sugar overrides" as shown in 12e7015. In the cases below all overrides are that of search() - a rather legitimate problem to be warning about Catalyst::Controller::DBIC::API DBIx::Class::Helpers DBIx::Class::ResultSet::AccessorsEverywhere No other known breakage as of this commit

To understand the process that lead to this commit, you'll probably need to read all of: http://lists.scsys.co.uk/pipermail/dbix-class/2016-October/date.html http://lists.scsys.co.uk/pipermail/dbix-class/2016-November/date.html http://lists.scsys.co.uk/pipermail/dbix-class/2016-December/date.html Any attempt on my part to summarise it would likely seem insufficiently accurate to me and biased to at least some readers, so I'm not even going to pretend to try. If you're trying to achieve a tl;dr, I suggest checking the December archive in the hopes that somebody posts a summary there some time after I push this commit.

Altreus · 2017-01-17T10:08:26Z

Any progress? Sorry to mither but it's holding us back because we're having to install this branch instead of latest

ribasushi · 2018-01-24T19:00:32Z

@Altreus terribly sorry you had to sit on this for so long. I just wanted to drop you a line that this is considered a blocker for the current master ( see label ) and won't be dropped on the floor. I will drop you another line once the more immediate maint-issues are taken care of, and dev from within master resumes.

Stay tuned, several weeks should be it.

Altreus force-pushed the people/altreus/unique-options branch from 05e063c to 22fc598 Compare November 3, 2015 13:45

Altreus force-pushed the people/altreus/unique-options branch 2 times, most recently from 92b3e21 to 0a6447f Compare November 3, 2015 14:44

ribasushi reviewed Nov 3, 2015
View reviewed changes

Altreus force-pushed the people/altreus/unique-options branch from e5ecb49 to 9596ab1 Compare November 3, 2015 15:55

ribasushi reviewed Nov 3, 2015
View reviewed changes

Altreus force-pushed the people/altreus/unique-options branch 2 times, most recently from 276b52b to 10a7334 Compare November 3, 2015 16:45

dbsrgits-sync force-pushed the current/blead branch from 4bc1f7b to 606ff39 Compare November 4, 2015 03:19

dbsrgits-sync force-pushed the current/blead branch from f2e955c to 1f99022 Compare January 20, 2016 13:04

ribasushi added 5 commits March 1, 2016 15:29

Stop loading Time::HiRes in the base test schema - there is no need

5a8d530

Switch all spots to a select()-based sleep instead

Several cosmetic fixups, making next commit easier to read

aff5e9c

No notable functional changes, mostly de-File::Spec-ing spots that do not need it ( '/' works on Win32 just fine, and VMS is a looooong way off )

(optdeps) One last pass through tests, streamline skip messages

3cff955

Do not load DBIC::Optional::Dependencies at runtime unless we need to

18a2903

ribasushi and others added 5 commits March 4, 2016 16:38

More robust behavior of ANFANG.pm, also guard against sitecustomize.pl

26710bc

Apparently -i and -jN do not work on win32, what the hell

1fb834d

These fixes together with RT#112706 make distbuilding on SighOS possible again Also adjust a number of whitespace checks to work properly, ensuring the distdir built on a normal OS and on Win32 match. Read under diff -w -C

These files are not used by anything - remnants from the distant ea2e61b

ad3a59b

(optdeps) _gen_pod can *very much* fail, not sure what I was thinking

7c29ff8

Fix erroneous use of multidimensional array emulation in 1fb834d

bd52af7

ribasushi and others added 22 commits September 27, 2016 10:21

Merge the relationship resolution rework

f4dc39d

Done as a merge to aid bisecting IFF it ever becomes necessary Wide testing indicates zero need for downstream changes (aside from several warnings)

Improve exception text during write operations on uninserted objects

84bf5d8

(travis) Work around RT#117959

89d5e36

A real fix for this ticket is pending, but had to be bumped a bit

Fix building on perls with no . in @inc

8aae794

Perl 5.26 will be able to be built with no . in @inc, and Debian are already building their 5.24 without it. To cope with this, do -It/lib -MFoo instead of -Mt::lib::Foo.

Tighten up select list processing in ::SQLMaker

02562a2

Optimize some of the codepaths (do not recurse in spots where it makes no practical difference). Deprecate searches with no explicit select-list ( can't remove it outright due to downstream breakage :/ )

Tighten up code in ResultSetColumns, add INDIRECT annotations

c5340bf

No functional changes (nothing else in lib/ and t/ had to change)

Restore the context sensitive m2m helper calling of ->search

13fd7cd

Subtly modified in 11e469d, this prevents things like the ::IgnoreWantarray helper from taking effect in this case. An audit of all other wantarray() invoking sites did not reveal other issues

Simplify guarded pass-through added to CDBI in ee33377

eab44f5

Simplify internal implementation of as_subselect_rs

9ab0364

Zero functional changes

Alastair McGowan-Douglas added 2 commits July 17, 2017 12:53

Allow SQLT options to be passed to unique constraints

9f29d1b

Test SQLT gets told about deferrable unique

fc0b714

Altreus force-pushed the people/altreus/unique-options branch from 7a04b66 to fc0b714 Compare July 17, 2017 11:53

ribasushi added the 0.082900 label Jan 24, 2018

ribasushi added 0.0828xx and removed 0.082900 labels Nov 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow SQLT options to be passed to unique constraints #88

Allow SQLT options to be passed to unique constraints #88

Altreus commented Nov 3, 2015

ribasushi commented Nov 3, 2015

Altreus commented Nov 3, 2015

Altreus commented Nov 3, 2015

ribasushi Nov 3, 2015

Altreus Nov 3, 2015

ribasushi Nov 3, 2015

Altreus commented Nov 3, 2015

ribasushi Nov 3, 2015

Altreus commented Nov 3, 2015

ribasushi commented Mar 3, 2016

Altreus commented Jan 17, 2017

ribasushi commented Jan 24, 2018

Allow SQLT options to be passed to unique constraints #88

Are you sure you want to change the base?

Allow SQLT options to be passed to unique constraints #88

Conversation

Altreus commented Nov 3, 2015

ribasushi commented Nov 3, 2015

Altreus commented Nov 3, 2015

Altreus commented Nov 3, 2015

ribasushi Nov 3, 2015

Choose a reason for hiding this comment

Altreus Nov 3, 2015

Choose a reason for hiding this comment

ribasushi Nov 3, 2015

Choose a reason for hiding this comment

Altreus commented Nov 3, 2015

ribasushi Nov 3, 2015

Choose a reason for hiding this comment

Altreus commented Nov 3, 2015

ribasushi commented Mar 3, 2016

Altreus commented Jan 17, 2017

ribasushi commented Jan 24, 2018