frioux / DBIx-Class-Helpers

https://metacpan.org/pod/DBIx::Class::Helpers
20 stars 38 forks source link

Make dont_serialize in Helper::Row::ToJSON a package global? #79

Closed kwakwaversal closed 7 years ago

kwakwaversal commented 7 years ago

Would be nice for https://github.com/frioux/DBIx-Class-Helpers/blob/master/lib/DBIx/Class/Helper/Row/ToJSON.pm#L12-L16 to be a package global, or expose "dont_serialize" in a sane manner to allow easily changing the default column types which shouldn't be serialised.

At $work we're using PostgreSQL, and using the text column instead of varchar (as they're synonymous). As such, all the text columns are being ignored by default.

Currently having to overload it directly which returns some of ToJSONs helpfulness to the boiler room. :( :)

use strict;
use warnings;

use base qw/DBIx::Class::Core/;

__PACKAGE__->load_components(qw/InflateColumn::DateTime Helper::Row::ToJSON/);

my $dont_serialize = {
        ntext => 1,
        blob  => 1,
};

sub _is_column_serializable {
   my ( $self, $column ) = @_;

   my $info = $self->column_info($column);

   if (!defined $info->{is_serializable}) {
      if (defined $info->{data_type} &&
          $dont_serialize->{lc $info->{data_type}}
      ) {
         $info->{is_serializable} = 0;
      } else {
         $info->{is_serializable} = 1;
      }
   }

   return $info->{is_serializable};
}

1;

Happy to help with a PR if you can let me know your preferred method to make this easier to implement.

frioux commented 7 years ago

I would much rather document how to subclass the package to make doing this clean but also still feel free per package. Does that make sense?

-- Sent from a telephone. Pardon my brevity.

On Jan 9, 2017 5:14 AM, "Paul Williams" notifications@github.com wrote:

Would be nice for https://github.com/frioux/DBIx-Class-Helpers/blob/ master/lib/DBIx/Class/Helper/Row/ToJSON.pm#L12-L16 to be a package global, or expose "dont_serialize" in a sane manner to allow easily changing the default column types which shouldn't be serialised.

At $work we're using PostgreSQL, and using the text column instead of varchar (as they're synonymous). As such, all the text columns are being ignored by default.

Currently having to overload it directly which returns some of ToJSONs helpfulness to the boiler room. :( :)

use strict;use warnings; use base qw/DBIx::Class::Core/; PACKAGE->load_components(qw/InflateColumn::DateTime Helper::Row::ToJSON/); my $dont_serialize = { ntext => 1, blob => 1, }; sub _is_columnserializable { my ( $self, $column ) = @;

my $info = $self->column_info($column);

if (!defined $info->{is_serializable}) { if (defined $info->{data_type} && $dont_serialize->{lc $info->{data_type}} ) { $info->{is_serializable} = 0; } else { $info->{is_serializable} = 1; } }

return $info->{is_serializable}; }

1;

Happy to help with a PR if you can let me know your preferred method to make this easier to implement.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/frioux/DBIx-Class-Helpers/issues/79, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAf4-TS4p9vEZsCXsl1VoU5zjxzmWszks5rQjK9gaJpZM4LePsC .

kwakwaversal commented 7 years ago

Right, that makes sense. I could cpanify a basic but functional example of this for postgres as DBIx::Class::Helper::Row::ToJSON::Pg perhaps?

package DBIx::Class::Helper::Row::ToJSON::Pg;

use strict;
use warnings;

use parent 'DBIx::Class::Helper::Row::ToJSON';

my $dont_serialize = {
  bytea    => 1,
  tsquery  => 1,
  tsvector => 1,
};

sub _is_column_serializable {
   my ( $self, $column ) = @_;

   my $info = $self->column_info($column);

   if (!defined $info->{is_serializable}) {
      if (defined $info->{data_type} &&
          $dont_serialize->{lc $info->{data_type}}
      ) {
         $info->{is_serializable} = 0;
      } else {
         $info->{is_serializable} = 1;
      }
   }

   return $info->{is_serializable};
}

1;

Returning the base result class to a simpler.

use strict;
use warnings;

use base qw/DBIx::Class::Core/;

__PACKAGE__->load_components(qw/InflateColumn::DateTime Helper::Row::ToJSON::Pg/);

1;
frioux commented 7 years ago

Right, though this isn't quite right; you'd have to subclass _is_column_serializeable and look at your list.

kwakwaversal commented 7 years ago

Ah. I've updated my comment.

I've just copied your _is_column_serializable method to also take advantage of utilising column_info.

I don't really like doing that because I would want any cpanified module to take advantage of any updates you might make in the future to DBIx::Class::Helper::Row::ToJSON. How would you feel about adding another method called unserializable_data_types (updating _is_column_serializable to probe this new method) which defaults to those in your $dont_serialize hash, but can be optionally overridden by the subclass? That would also allow users to allow certain restricted data types per result in a similar fashion to them overriding serializable_columns?

package DBIx::Class::Helper::Row::ToJSON::Pg;

use strict;
use warnings;

use parent 'DBIx::Class::Helper::Row::ToJSON';

my $dont_serialize = {
  bytea    => 1,
  tsquery  => 1,
  tsvector => 1,
};

sub unserializable_data_types {
  return $dont_serialize;
}

1;

Or should I just suck it up? :) I don't mind creating a PR for the work if it's something you're OK with.

frioux commented 7 years ago

Yeah I like the idea of the new method. Send me a PR with that and I'll take it from there.

kwakwaversal commented 7 years ago

Let me know if there's anything else I can do to help with this. I was reminded about it because I'm using it again in another application, so currently throwing in some repeated code.

Cheers.