renatomassaro
commented
7 years ago Now it depends on Cache (#231)
Closed renatomassaro closed 7 years ago
renatomassaro
commented
7 years ago Now it depends on Cache (#231)
umamaistempo
commented
7 years ago Reviewed 12 of 37 files at r1, 51 of 70 files at r2. Review status: 63 of 79 files reviewed at latest revision, 75 unresolved discussions.
lib/cache/internal/purge.ex, line 87 at r2 (raw file):
end defp delete(:network, {network_id, ip}) do NetworkCache.Query.by_nip(network_id, ip)
pipeline
lib/cache/internal/purge.ex, line 91 at r2 (raw file):
end defp delete(:web, {network_id, ip}) do WebCache.Query.web_by_nip(network_id, ip)
pipeline
lib/cache/model/cacheable.ex, line 166 at r2 (raw file):
|> WebCache.create_changeset() |> Ecto.Changeset.apply_changes() end
This receives a %WebCache{}, inputs it to a changeset of webcache and then returns it again. This does not really makes sense, probably old code
(Same applies to other impls of this protocol.function
lib/cache/query/cache.ex, line 44 at r2 (raw file):
@spec from_server_get_storages(Server.idtb) :: {:ok, [Storage.idtb]}
I don't think this is apropriate. This means that this function returns a maybe_empty_list whose contents are Storage.t and/or Storage.id and/or binary. Obviously a function like this should return only one of the three (personally i think it should return the Storage.id or Storage.t but not a mix of both)
lib/cache/query/cache.ex, line 64 at r2 (raw file):
@spec from_server_get_components(Server.idtb) :: {:ok, [Component.idtb]}
same comment as from_server_get_storages/1
lib/cache/query/cache.ex, line 86 at r2 (raw file):
@spec from_motherboard_get_entity(Motherboard.idtb) :: {:ok, Entity.idtb}
same comment as from_server_get_storages/1
lib/cache/query/cache.ex, line 112 at r2 (raw file):
@spec from_motherboard_get_components(Motherboard.idtb) :: {:ok, [Component.idtb]}
same comment as from_server_get_storages/1
lib/cache/query/cache.ex, line 125 at r2 (raw file):
@spec from_entity_get_motherboard(Entity.idtb) :: {:ok, Motherboard.idtb}
same comment as from_server_get_storages/1
lib/cache/query/cache.ex, line 136 at r2 (raw file):
@spec from_storage_get_server(Storage.idtb) :: {:ok, Server.idtb}
same comment as from_server_get_storages/1
lib/cache/query/cache.ex, line 148 at r2 (raw file):
@spec from_nip_get_server(Network.idtb, IPv4.t) :: {:ok, Server.idtb}
same comment as from_server_get_storages/1
lib/cache/query/cache.ex, line 172 at r2 (raw file):
@spec from_component_get_motherboard(Component.idtb) :: {:ok, Motherboard.idtb}
same comment as from_server_get_storages/1
lib/entity/action/entity.ex, line 32 at r2 (raw file):
def create_from_specialization(%NPC{npc_id: npc_id}) do params = %{ entity_id: npc_id,
this is going to break.
EntityInternal.create/1 expects an input that contains an Entity.idtb as the entity_id field; the NPC.npc_id field is an %NPC.ID{} so it can't be cast (without a helper) to an Entity.ID.
My suggestion: either use to_string just like the account create_from_specialization/1 clause above or use EntityQuery.get_entity_id/1 to cast it to an entity id
lib/entity/query/entity.ex, line 71 at r2 (raw file):
%Entity.ID{id: id} _ -> entity
This is wrong. It will break the contract because it will return a value that is not an entity id should a non-valid value be input.
My suggestion:
case entity do
%Account{account_id: %Account.ID{id: id}} ->
%Entity.ID{id: id}
%Account.ID{id: id} ->
%Entity.ID{id: id}
%NPC{npc_id: %NPC.ID{id: id}} ->
%Entity.ID{id: id}
%NPC.ID{id: id} ->
%Entity.ID{id: id}
value ->
Entity.ID.cast!(value)
endlib/hell/hell/ip.ex, line 129 at r2 (raw file):
{:ok, Postgrex.INET.t} | :error def parse_address(string) when is_binary(string) do
I don't think this should be exported
lib/hell/hell/map.ex, line 2 at r2 (raw file):
defmodule HELL.MapUtils do
no linebreak between defmodule and @moduledoc
lib/hell/hell/map.ex, line 10 at r2 (raw file):
Convert map string keys to :atom keys """ def atomize_keys(nil), do: nil
this clause doesn't make too much sense since the last clause is already a match all
lib/hell/hell/map.ex, line 14 at r2 (raw file):
# Structs don't do enumerable and anyway the keys are already # atoms def atomize_keys(struct = %{__struct__: _}) do
this is correct but i'd prefer the pattern struct = %_{} here because it looks nicer :)
lib/hell/hell/map.ex, line 23 at r2 (raw file):
{k, v} when is_atom(k) -> {k, atomize_keys(v)} {k, v} ->
Technically erlang maps can have any erlang term as key. since you're expecting to always have a %{String.t | atom => any} map as input, i think this is okay
lib/hell/hell/map.ex, line 24 at r2 (raw file):
{k, atomize_keys(v)} {k, v} -> {String.to_atom(k), atomize_keys(v)}
not good, remember that this might bloat the atom table
lib/hell/hell/map.ex, line 26 at r2 (raw file):
{String.to_atom(k), atomize_keys(v)} end) |> Enum.into(%{})
I'd prefer :maps.from_list/1 because it executes directly the NIF that builds a map from a list of 2-tuple
lib/hell/hell/pk.ex, line 47 at r2 (raw file):
{:ok, Postgrex.INET.t} | :error def parse_address(string) when is_binary(string) do
I don't think this should be exported
lib/hell/hell/pk/header.ex, line 15 at r2 (raw file):
account_account: [0x0001, 0x0000, 0x0000], universe_npc:
I think this file is not used anymore :P
With the inclusion of HELL.PK this was deprecated. Going to remove it in another PR
lib/network/action/dns.ex, line 14 at r2 (raw file):
{:ok, Unicast.t} | {:error, Ecto.Changeset.t} def register_unicast(network, name, ip) do
i think you should accept either Network.t or Network.idt here
lib/network/action/dns.ex, line 20 at r2 (raw file):
@spec deregister_unicast(Network.id, String.t) :: :ok def deregister_unicast(network, name),
i think you should accept either Network.t or Network.idt here
lib/network/action/dns.ex, line 27 at r2 (raw file):
| {:error, Ecto.Changeset.t} def register_anycast(name, npc_id) do if NPCQuery.fetch(npc_id) do
if you want to ensure that a valid npc is input, require it as a NPC.t (ie: npc = %NPC{})
lib/network/internal/dns.ex, line 11 at r2 (raw file):
Unicast.t | nil def lookup_unicast(network, name) do
i think you should accept either Network.t or Network.idt here
lib/network/internal/dns.ex, line 14 at r2 (raw file):
name |> Unicast.Query.by_name(network) |> Repo.one
parens
lib/network/internal/dns.ex, line 23 at r2 (raw file):
name |> Anycast.Query.by_name() |> Repo.one
parens
lib/network/internal/dns.ex, line 32 at r2 (raw file):
params |> Unicast.create_changeset() |> Repo.insert
parens
lib/network/internal/dns.ex, line 37 at r2 (raw file):
@spec deregister_unicast(Network.id, String.t) :: :ok def deregister_unicast(network, name) do
i think you should accept either Network.t or Network.idt here
lib/network/internal/web.ex, line 11 at r2 (raw file):
| :notfound def serve(network, ip) do case CacheQuery.from_nip_get_web(network, ip) do
Shouldn't the access to CacheQuery exist only on Action | Query modules ?
lib/network/model/dns/anycast.ex, line 14 at r2 (raw file):
@type creation_params :: %{ :name => Constant.t, :npc_id => NPC.id
I'd say this aceps NPC.idtb since, as we talked before, the higher layers restrict the input and the lower layers accept anything (ie: the Action should restrict - if useful - the input to NPC.t, the Internal to NPC.idt and the model to NPC.idtb) but this is not really essential
lib/network/model/dns/anycast.ex, line 22 at r2 (raw file):
@primary_key false
remove this linebreak
lib/network/model/dns/anycast.ex, line 53 at r2 (raw file):
def by_npc(query \\ Anycast, npc), do: where(query, [a], a.npc_id == ^npc) end
alias Queryable to use shorter types :P
lib/network/model/dns/unicast.ex, line 14 at r2 (raw file):
@type creation_params :: %{ :name => String.t, :network_id => Network.id,
same as with Anycast model
lib/network/model/dns/unicast.ex, line 23 at r2 (raw file):
@primary_key false
same as with Anycast model
lib/network/model/dns/unicast.ex, line 56 at r2 (raw file):
def by_nip(query \\ Unicast, network, ip), do: where(query, [u], u.network_id == ^network and u.ip == ^ip) end
same as with Anycast model (alias Queryable)
lib/network/model/web/player.ex, line 1 at r2 (raw file):
defmodule Helix.Network.Model.Web.Player do
why is it called player ?
lib/network/model/web/player.ex, line 25 at r2 (raw file):
field :content, :string, size: 2048
this param does not exist and is ignored. If you want to check for a max size you should either add a constraint in the migration or add a validate_length/3 call on the create_changeset/1
lib/network/model/web/player.ex, line 31 at r2 (raw file):
%__MODULE__{} |> cast(params, @creation_fields) |> validate_required([:content])
why isn't ip required ?
Note tho that requiring content means that it cannot be an empty string.
ie: %{content: ""} will add an error to the changeset
lib/network/model/web/player.ex, line 45 at r2 (raw file):
def by_ip(query \\ Player, ip), do: where(query, [w], w.ip == ^ip) end
alias Ecto.Queryable
lib/network/query/dns.ex, line 11 at r2 (raw file):
alias Helix.Network.Model.Network @spec resolve(Network.idtb, String.t, IPv4.t) ::
I think the query should receive either Network.t or Network.idt
lib/network/query/dns.ex, line 31 at r2 (raw file):
true -> :nxdomain
Not a good return (it's an okay || :unique_error_atom for an with clause tho). Use :error or {:error, atom}
lib/network/query/dns.ex, line 35 at r2 (raw file):
end @spec nearest_server(Network.idtb, NPC.id, IPv4.t) ::
I think the query should receive either Network.t or Network.idt
lib/network/query/dns.ex, line 42 at r2 (raw file):
npc_id |> NPCQuery.fetch() |> EntityQuery.get_entity_id()
Why are you fetching an NPC to get the entity ? If it is to ensure that the NPC exists, don't do it as it whould be implied as true by the Anycast record having it's id (aka let it crash)
lib/network/query/web.ex, line 11 at r2 (raw file):
alias Helix.Network.Query.DNS, as: DNSQuery @spec browse(Network.idtb, (String.t | IPv4.t), IPv4.t) ::
I think the query should receive either Network.t or Network.idt
also, why are you putting parens on String.t | IPv4.t ? There is no ambiguity there to require it
lib/network/query/web.ex, line 12 at r2 (raw file):
@spec browse(Network.idtb, (String.t | IPv4.t), IPv4.t) :: {:ok, {:npc | :vpc, term}}
ain't the success return {:ok, {:npc | :vpc, String.t}} ?
lib/network/query/web.ex, line 16 at r2 (raw file):
| {:error, :nxdomain} def browse(network, address, origin) do case IPv4.parse_address(address) do
IPv4.parse_address/1 should be a private function to convert an autogenerated input to the valid ecto type.
Instead create a public function IPv4.valid?/1
lib/network/query/web.ex, line 24 at r2 (raw file):
browse_ip(network, ip) :nxdomain -> {:error, :nxdomain}
i don't think the atom nxdomain is very good, i'd prefer something like {:error, {:domain, :notfound}} as it gives a more clear and direct error (that could be pattern matched with other possible future domain related errors)
lib/network/query/web.ex, line 29 at r2 (raw file):
end @spec browse_ip(Network.idtb, IPv4.t) ::
I think the query should receive either Network.t or Network.idt
lib/network/query/web.ex, line 30 at r2 (raw file):
@spec browse_ip(Network.idtb, IPv4.t) :: {:ok, {:npc | :vpc, term}}
ain't the success return {:ok, {:npc | :vpc, String.t}} ?
lib/universe/npc/internal/web.ex, line 12 at r2 (raw file):
} @spec generate_content(NPC.t, Network.id, IPv4.t) ::
I think this should accept an Network.idt
lib/universe/npc/model/npc.ex, line 9 at r2 (raw file):
alias Helix.Universe.NPC.Model.NPCType import Ecto.Changeset
I think we're using the following order: use > import > alias
lib/universe/npc/model/npc.ex, line 15 at r2 (raw file):
inserted_at: NaiveDateTime.t, updated_at: NaiveDateTime.t }
what abount npc_type ?
lib/universe/npc/model/npc.ex, line 52 at r2 (raw file):
def by_id(query \\ NPC, npc), do: where(query, [n], n.npc_id == ^npc) end
alias queryable
priv/repo/network/migrations/20170712002537_dns.exs, line 10 at r2 (raw file):
create table(:dns_unicast, primary_key: false) do add :name, :string, primary_key: true add :network_id, :inet, primary_key: true
This is wrong. The PK should be {network, hostname}, not {hostname, network}
priv/repo/network/migrations/20170712002537_dns.exs, line 11 at r2 (raw file):
add :name, :string, primary_key: true add :network_id, :inet, primary_key: true add :ip, :inet
i think this should not be nullable
priv/repo/network/migrations/20170712044302_webserver.exs, line 15 at r2 (raw file):
add :content, :map, null: false add :expiration_time, :utc_datetime, default: fragment("now()") end
why is this here and not on cache ?
Also, i grepped lib and it seems this table is not being used
test/cache/action/cache_test.exs, line 201 at r2 (raw file):
describe "update_network/1" do test "update_network/1", context do
erm what ?
test/cache/action/cache_test.exs, line 214 at r2 (raw file):
assert StatePurgeQueue.lookup(:storage, storage1.storage_id) assert StatePurgeQueue.lookup(:component, mobo1.motherboard_id) Enum.map(server1.components, fn(component_id) ->
You're traversing the input to cause a side-effect (and don't care about it's return). You should not map it.
Use either Enum.each(server1.components, &(assert StatePurgeQueue.lookup(:component, &1)) or assert Enum.all?(server1.components &StatePurgeQueue.lookup(:component, &1))
test/server/query/server_test.exs, line 44 at r2 (raw file):
{server, _} = Setup.server() ip = ServerQuery.get_ip(server.server_id, NetworkHelper.internet_id)
parens on function call
test/support/id_case.ex, line 12 at r2 (raw file):
assert_id v, Map.get(b, k) end) end
It's not clear what this exactly does and i don't believe this belongs to this helper
test/support/id_case.ex, line 18 at r2 (raw file):
assert_id(a, b) end) end
I don't think this clause is really ideal. I think assert_id should only accept a single id and compare it with another single id
test/support/id_case.ex, line 21 at r2 (raw file):
def assert_id(a, b) do assert to_string(a) == to_string(b) end
spaces between each clause
test/support/hell/setup.ex, line 16 at r2 (raw file):
{server, account} end
remove this linebreak
test/universe/npc/internal/npc_test.exs, line 21 at r2 (raw file):
describe "fetching" do test "success by id" do npc = NPCHelper.random()
i think you should use a factory to insert an npc like on other tests
test/universe/npc/internal/npc_test.exs, line 25 at r2 (raw file):
end test "fails when npc with id doesn't exist" do
fails when npc doesn't exist
test/universe/npc/internal/npc_test.exs, line 28 at r2 (raw file):
bogus = Random.pk() refute NPCInternal.fetch(bogus) end
On the other domains we are expecting fetch to receive an id term instead of a binary input. Use the id generation function
test/universe/npc/internal/npc_test.exs, line 32 at r2 (raw file):
test "delete/1" do npc_id = NPCHelper.random().id
i think you should use a factory to insert an npc like on other tests
test/universe/npc/internal/npc_test.exs, line 36 at r2 (raw file):
assert npc NPCInternal.delete(npc) refute NPCInternal.fetch(npc_id)
add some spaces between the phases of your test
test/universe/npc/query/npc_test.exs, line 12 at r2 (raw file):
describe "fetch/1" do test "it fetches (npc id)" do
no need for the it part. prefer test "accepts npc id"
test/universe/npc/query/npc_test.exs, line 13 at r2 (raw file):
describe "fetch/1" do test "it fetches (npc id)" do npc = NPCHelper.random()
i think you should use a factory to insert an npc like on other tests
test/universe/npc/query/npc_test.exs, line 17 at r2 (raw file):
end test "it fetches (entity id)" do
no need for the it part. prefer test "accepts entity id"
test/universe/npc/query/npc_test.exs, line 23 at r2 (raw file):
end test "it won't fetch stuff that does not exist" do
no need for the it part. prefer test "fails if npc doesn't exists"
test/universe/npc/query/npc_test.exs, line 24 at r2 (raw file):
test "it won't fetch stuff that does not exist" do refute NPCQuery.fetch(NPC.ID.cast!(Random.pk()))
Why don't you just NPC.ID.generate() instead ?
Comments from Reviewable
umamaistempo
commented
7 years ago Reviewed 11 of 70 files at r2. Review status: 74 of 79 files reviewed at latest revision, 100 unresolved discussions.
lib/hell/hell/pk/header.ex, line 15 at r2 (raw file):
I think this file is not used anymore :P With the inclusion of `HELL.PK` this was deprecated. Going to remove it in another PR
HELL.ID, not HELL.PKlib/universe/npc/seed.ex, line 27 at r2 (raw file):
add_npc_types() Enum.map(npcs, fn (entry) ->
You're not using the return, so Enum.each should be used
lib/universe/npc/seed.ex, line 35 at r2 (raw file):
|> Ecto.Changeset.cast(%{npc_id: entry.id}, [:npc_id]) |> Repo.insert() end
instead of this check, simply do a Repo.insert(on_conflict: :nothing)
lib/universe/npc/seed.ex, line 58 at r2 (raw file):
# Give time to commit previous transactions :timer.sleep(500)
No need to sleep. A Repo transaction is blocking (ie: this code will only be executed after the transaction is committed/rollbacked)
lib/universe/npc/seed.ex, line 73 at r2 (raw file):
# Create Server server = %{server_type: :desktop}
start on next line
lib/universe/npc/seed.ex, line 76 at r2 (raw file):
|> Server.create_changeset() |> Ecto.Changeset.cast(%{server_id: entry_server.id}, [:server_id]) |> ServerRepo.insert!
parens
lib/universe/npc/seed.ex, line 87 at r2 (raw file):
# Change IP if a static one was specified if entry_server.static_ip do nc = motherboard_id
start on next line
lib/universe/npc/seed.ex, line 100 at r2 (raw file):
def create_dns(entry, npc) do if entry.anycast do
if entry.anycast && !DNSInternal.lookup_anycast(entry.anycast) do
lib/universe/npc/seed.ex, line 112 at r2 (raw file):
# load spikes on production. Filter out to purge only servers who were added # during the migration. Enum.map(npcs, fn(npc) ->
Enum.each
lib/universe/npc/seed.ex, line 113 at r2 (raw file):
# during the migration. Enum.map(npcs, fn(npc) -> Enum.map(npc.servers, fn(server) ->
Enum.each
test/cache/query/cache_test.exs, line 192 at r2 (raw file):
end describe "from_nip_get_web" do
describe "from_nip_get_web/2"
test/cache/query/cache_test.exs, line 193 at r2 (raw file):
describe "from_nip_get_web" do test "it works" do
test "returns webserver bound to nip"
test/hardware/internal/network_connection_test.exs, line 42 at r2 (raw file):
test "won't update to an existing ip", context do server_id = context.server.server_id existing_ip = "1.2.3.4"
I'd recomend not assuming a hardcoded entry, instead generating a random ip and assigning it, so you can be sure that, even if the seed is changed (and it will), this test will still pass
test/hardware/internal/network_connection_test.exs, line 60 at r2 (raw file):
describe "fetch/1" do test "it fetches, just like phoebe", context do
test "returns the specified network connection"
test/hardware/internal/network_connection_test.exs, line 73 at r2 (raw file):
end describe "fetch_by_nip" do
describe "fetch_by_nip/2"
test/hardware/internal/network_connection_test.exs, line 74 at r2 (raw file):
describe "fetch_by_nip" do test "it works", context do
test "returns the network connection bound to specified nip"
test/hardware/internal/network_connection_test.exs, line 81 at r2 (raw file):
nc = NetworkConnectionInternal.fetch_by_nip(nip.network_id, nip.ip) refute nc == nil
assert nc
test/network/action/dns_test.exs, line 38 at r2 (raw file):
assert {:ok, _} = DNSAction.register_unicast(network1, site1, ip1) assert_raise Ecto.ConstraintError, fn -> DNSAction.register_unicast(network1, site1, ip2)
Add a comment to make it obvious that the reason such error is raised is because on an unicast you tried to register two different ips (and an unicast can only point to a single ip)
test/network/action/dns_test.exs, line 46 at r2 (raw file):
describe "deregister_unicast/2" do test "it removes unicast entry" do
test "removes the entry" or removes unicast entry or removes specified entry
test/network/internal/web_test.exs, line 12 at r2 (raw file):
{_, ip} = NPCHelper.download_center() assert {:ok, page} = WebInternal.serve("::", ip)
Since internet is a constant, i'd prefer to use the helper (and avoid assuming that the internal accepts binary id)
test/network/query/dns_test.exs, line 28 at r2 (raw file):
test "Unicast resolution" do {site, ip} = {"saocarlosagora.com.br", Random.ipv4()}
why are you binding this way ?
prefer two separate lines since you're not using the boxed type for anything else
test/network/query/dns_test.exs, line 41 at r2 (raw file):
"wwwwwwwww.jodi.org", Random.ipv4()) end
lacking an assertion.
test/network/query/web_test.exs, line 11 at r2 (raw file):
describe "browse/3" do test "it browses with ip" do
test "accepts ip"
test/network/query/web_test.exs, line 20 at r2 (raw file):
end test "it browses with name" do
test "accepts hostname"
test/network/query/web_test.exs, line 29 at r2 (raw file):
end test "it wont find non-existing ip" do
test "fails when ip doesn't exist"
test/network/query/web_test.exs, line 34 at r2 (raw file):
end test "it wont find non-existing domain" do
test "fails when domain doesn't exist"
test/server/query/server_test.exs, line 46 at r2 (raw file):
ip = ServerQuery.get_ip(server.server_id, NetworkHelper.internet_id) assert ip
why are you binding instead of directly asserting the function ?
(ie: you're not using the binding for anything else and it doesn't breaks the column limit)
test/support/npc_helper.ex, line 43 at r2 (raw file):
# Remove potentially cached data CacheHelper.empty_cache() end
I understand the CacheHelper.empty_cache() but i can't understand why you're deleting everything else when we have migrations that ensure that the database contains the basic environment and we even use sandbox so the database state is always clean
Comments from Reviewable
renatomassaro
commented
7 years ago Review status: 74 of 79 files reviewed at latest revision, 101 unresolved discussions.
lib/cache/internal/purge.ex, line 87 at r2 (raw file):
pipeline
?
lib/cache/internal/purge.ex, line 91 at r2 (raw file):
pipeline
?
lib/cache/model/cacheable.ex, line 166 at r2 (raw file):
This receives a `%WebCache{}`, inputs it to a changeset of webcache and then returns it again. This does not really makes sense, probably old code (Same applies to other impls of this protocol.function
Actually, it does change the data set. Just tested here.
Basically, create_changeset will cast HELL.ID to HELL.PK (binary), which Cache uses internally (format_input is used to receive data from Helix, so this conversion happens there).
In some cases such conversion does not happen, as is the case for WebCache, because if you see at builder.ex, the only function it receives as input are nip and content. Nip is already internal to cache, so it already is saved as PK.t. Content does not have an ID so nothing happens.
Contrast it to, say, Storage. It adds as input storage_id and server_id. storage_id comes from cache internally, so it's already in PK.t, but server_id comes from Helix and must be casted to PK.t.
WebCache is the only one who does not need to convert Helix ID to PK.t... BUT I'll leave the function there, since it's the expected behavior that whatever internal ID I throw at the Model, it will cast from ID to PK.
Sample output:
before
%Helix.Cache.Model.ServerCache{__meta__: #Ecto.Schema.Metadata<:built, "server_cache">,
components: [%Helix.Hardware.Model.Component.ID{id: {17, 0, 0, 30882, 31197,
33120, 3786, 60482}, root: Helix.Hardware.Model.Component},
%Helix.Hardware.Model.Component.ID{id: {17, 0, 0, 33954, 42799, 15611, 26426,
54429}, root: Helix.Hardware.Model.Component},
%Helix.Hardware.Model.Component.ID{id: {17, 0, 0, 54586, 17895, 25973, 6768,
65403}, root: Helix.Hardware.Model.Component},
%Helix.Hardware.Model.Component.ID{id: {17, 0, 0, 10484, 37183, 24985, 13406,
21059}, root: Helix.Hardware.Model.Component}],
entity_id: %Helix.Entity.Model.Entity.ID{id: {1, 0, 0, 17481, 37268, 64059,
21863, 2930}, root: Helix.Entity.Model.Entity}, expiration_date: nil,
motherboard_id: %Helix.Hardware.Model.Component.ID{id: {17, 0, 0, 34922, 33461,
32944, 6682, 37167}, root: Helix.Hardware.Model.Component},
networks: [%{ip: "69.151.89.119",
network_id: %Helix.Network.Model.Network.ID{id: {0, 0, 0, 0, 0, 0, 0, 0},
root: Helix.Network.Model.Network}}],
resources: %{cpu: 1333, hdd: 1024,
net: %{"::" => %{downlink: 100, uplink: 100}}, ram: 1024},
server_id: "10::2fe9:45dd:b807:57a0:a7ff",
storages: [%Helix.Software.Model.Storage.ID{id: {32, 1, 0, 48322, 49514, 44211,
49644, 1372}, root: Helix.Software.Model.Storage}]}after
%Helix.Cache.Model.ServerCache{__meta__: #Ecto.Schema.Metadata<:built, "server_cache">,
components: ["11::78a2:79dd:8160:eca:ec42", "11::84a2:a72f:3cfb:673a:d49d",
"11::d53a:45e7:6575:1a70:ff7b", "11::28f4:913f:6199:345e:5243"],
entity_id: "1::4449:9194:fa3b:5567:b72",
expiration_date: #DateTime<2017-08-12 06:02:07.590Z>,
motherboard_id: "11::886a:82b5:80b0:1a1a:912f",
networks: [%{ip: "69.151.89.119", network_id: "::"}],
resources: %{cpu: 1333, hdd: 1024,
net: %{"::" => %{downlink: 100, uplink: 100}}, ram: 1024},
server_id: "10::2fe9:45dd:b807:57a0:a7ff",
storages: ["20:1:0:bcc2:c16a:acb3:c1ec:55c"]}lib/cache/query/cache.ex, line 44 at r2 (raw file):
I don't think this is apropriate. This means that this function returns a maybe_empty_list whose contents are `Storage.t` and/or `Storage.id` and/or `binary`. Obviously a function like this should return only one of the three (personally i think it should return the `Storage.id` or `Storage.t` but not a mix of both)
Actually it is only .id
lib/cache/query/cache.ex, line 64 at r2 (raw file):
same comment as `from_server_get_storages/1`
Done.
lib/cache/query/cache.ex, line 86 at r2 (raw file):
same comment as `from_server_get_storages/1`
Done.
lib/cache/query/cache.ex, line 112 at r2 (raw file):
same comment as `from_server_get_storages/1`
Done.
lib/cache/query/cache.ex, line 125 at r2 (raw file):
same comment as `from_server_get_storages/1`
Done.
lib/cache/query/cache.ex, line 136 at r2 (raw file):
same comment as `from_server_get_storages/1`
Done.
lib/cache/query/cache.ex, line 148 at r2 (raw file):
same comment as `from_server_get_storages/1`
Done.
lib/cache/query/cache.ex, line 172 at r2 (raw file):
same comment as `from_server_get_storages/1`
Done.
lib/entity/action/entity.ex, line 32 at r2 (raw file):
this is going to break. `EntityInternal.create/1` expects an input that contains an `Entity.idtb` as the `entity_id` field; the `NPC.npc_id` field is an `%NPC.ID{}` so it can't be cast (without a helper) to an `Entity.ID`. My suggestion: either use `to_string` just like the account `create_from_specialization/1` clause above or use `EntityQuery.get_entity_id/1` to cast it to an entity id
Done.
lib/entity/query/entity.ex, line 71 at r2 (raw file):
This is wrong. It will break the contract because it will return a value that is not an entity id should a non-valid value be input. My suggestion: ``` case entity do %Account{account_id: %Account.ID{id: id}} -> %Entity.ID{id: id} %Account.ID{id: id} -> %Entity.ID{id: id} %NPC{npc_id: %NPC.ID{id: id}} -> %Entity.ID{id: id} %NPC.ID{id: id} -> %Entity.ID{id: id} value -> Entity.ID.cast!(value) end ```
:+1: I've changed the value catch-all to two explicit Entity conditions. Any other calls should crash (e.g. when we add clan).
lib/hell/hell/ip.ex, line 129 at r2 (raw file):
I don't think this should be exported
I need a string -> inet parser. If not this one, then I'll have to add some external lib that does exactly this.
lib/hell/hell/map.ex, line 2 at r2 (raw file):
no linebreak between `defmodule` and `@moduledoc`
Done.
lib/hell/hell/map.ex, line 10 at r2 (raw file):
this clause doesn't make too much sense since the last clause is already a match all
Done.
lib/hell/hell/map.ex, line 14 at r2 (raw file):
this is correct but i'd prefer the pattern `struct = %_{}` here because it looks nicer :)
Done.
lib/hell/hell/map.ex, line 24 at r2 (raw file):
not good, remember that this might bloat the atom table
atomize_keys should only be use for safe structs, and that's the case. I've changed it to to_existing_atoms.
lib/hell/hell/map.ex, line 26 at r2 (raw file):
I'd prefer `:maps.from_list/1` because it executes directly the NIF that builds a map from a list of 2-tuple
Done.
lib/hell/hell/pk.ex, line 47 at r2 (raw file):
I don't think this should be exported
Done.
lib/hell/hell/pk/header.ex, line 15 at r2 (raw file):
* `HELL.ID`, not `HELL.PK`
Yep, not used, but do not remove it yet! It's the only place we document our ID's ids.
lib/network/action/dns.ex, line 14 at r2 (raw file):
i think you should accept either `Network.t` or `Network.idt` here
Done.
lib/network/action/dns.ex, line 20 at r2 (raw file):
i think you should accept either `Network.t` or `Network.idt` here
Done.
lib/network/action/dns.ex, line 27 at r2 (raw file):
if you want to ensure that a valid npc is input, require it as a `NPC.t` (ie: `npc = %NPC{}`)
Done.
lib/network/internal/dns.ex, line 11 at r2 (raw file):
i think you should accept either `Network.t` or `Network.idt` here
Done.
lib/network/internal/dns.ex, line 14 at r2 (raw file):
parens
Done.
lib/network/internal/dns.ex, line 23 at r2 (raw file):
parens
Done.
lib/network/internal/dns.ex, line 32 at r2 (raw file):
parens
Done.
lib/network/internal/dns.ex, line 37 at r2 (raw file):
i think you should accept either `Network.t` or `Network.idt` here
Done.
lib/network/internal/web.ex, line 11 at r2 (raw file):
Shouldn't the access to `CacheQuery` exist only on `Action | Query` modules ?
Done.
lib/network/model/dns/anycast.ex, line 14 at r2 (raw file):
I'd say this aceps `NPC.idtb` since, as we talked before, the higher layers restrict the input and the lower layers accept anything (ie: the `Action` should restrict - if useful - the input to `NPC.t`, the `Internal` to `NPC.idt` and the model to `NPC.idtb`) but this is not _really_ essential
Done.
lib/network/model/dns/anycast.ex, line 22 at r2 (raw file):
remove this linebreak
Done.
lib/network/model/dns/anycast.ex, line 53 at r2 (raw file):
alias Queryable to use shorter types :P
Done.
lib/network/model/dns/unicast.ex, line 14 at r2 (raw file):
same as with Anycast model
Done.
lib/network/model/dns/unicast.ex, line 23 at r2 (raw file):
same as with Anycast model
Done.
lib/network/model/dns/unicast.ex, line 56 at r2 (raw file):
same as with Anycast model (alias Queryable)
Done.
lib/network/model/web/player.ex, line 1 at r2 (raw file):
why is it called player ?
Because it belongs to the player, and only to the player. NPC and Clans have different handlers
lib/network/model/web/player.ex, line 25 at r2 (raw file):
this param does not exist and is ignored. If you want to check for a max size you should either add a constraint in the migration or add a `validate_length/3` call on the `create_changeset/1`
Done.
lib/network/model/web/player.ex, line 31 at r2 (raw file):
why isn't `ip` required ? Note tho that requiring `content` means that it cannot be an empty string. ie: `%{content: ""}` will add an error to the changeset
Done.
lib/network/model/web/player.ex, line 45 at r2 (raw file):
alias Ecto.Queryable
Done.
lib/network/query/dns.ex, line 11 at r2 (raw file):
I think the query should receive either `Network.t` or `Network.idt`
Done.
lib/network/query/dns.ex, line 31 at r2 (raw file):
Not a good return (it's an okay `|| :unique_error_atom` for an with clause tho). Use `:error` or `{:error, atom}`
Done.
lib/network/query/dns.ex, line 35 at r2 (raw file):
I think the query should receive either `Network.t` or `Network.idt`
Done.
lib/network/query/dns.ex, line 42 at r2 (raw file):
Why are you fetching an NPC to get the entity ? If it is to ensure that the NPC exists, don't do it as it whould be implied as true by the Anycast record having it's id (aka let it crash)
Done.
lib/network/query/web.ex, line 11 at r2 (raw file):
I think the query should receive either `Network.t` or `Network.idt` also, why are you putting parens on `String.t | IPv4.t` ? There is no ambiguity there to require it
Done.
lib/network/query/web.ex, line 12 at r2 (raw file):
ain't the success return `{:ok, {:npc | :vpc, String.t}}` ?
Nope. It's a map with the web server info, which is quite custom for npcs. Once we have a better idea of what it looks like, we can specify a better type (something like: web_npc | web_vpc | web_clan)
lib/network/query/web.ex, line 16 at r2 (raw file):
`IPv4.parse_address/1` should be a private function to convert an autogenerated input to the valid ecto type. Instead create a public function `IPv4.valid?/1`
Done.
lib/network/query/web.ex, line 24 at r2 (raw file):
i don't think the atom `nxdomain` is very good, i'd prefer something like `{:error, {:domain, :notfound}}` as it gives a more clear and direct error (that could be pattern matched with other possible future domain related errors)
Done.
lib/network/query/web.ex, line 29 at r2 (raw file):
I think the query should receive either `Network.t` or `Network.idt`
Done.
lib/network/query/web.ex, line 30 at r2 (raw file):
ain't the success return `{:ok, {:npc | :vpc, String.t}}` ?
Done.
lib/universe/npc/seed.ex, line 27 at r2 (raw file):
You're not using the return, so `Enum.each` should be used
Done.
lib/universe/npc/seed.ex, line 35 at r2 (raw file):
instead of this check, simply do a `Repo.insert(on_conflict: :nothing)`
Done.
lib/universe/npc/seed.ex, line 58 at r2 (raw file):
No need to sleep. A Repo transaction is blocking (ie: this code will only be executed after the transaction is committed/rollbacked)
Actually that was for the cache
lib/universe/npc/seed.ex, line 73 at r2 (raw file):
start on next line
Done.
lib/universe/npc/seed.ex, line 76 at r2 (raw file):
parens
Done.
lib/universe/npc/seed.ex, line 87 at r2 (raw file):
start on next line
Done.
lib/universe/npc/seed.ex, line 100 at r2 (raw file):
`if entry.anycast && !DNSInternal.lookup_anycast(entry.anycast) do`
That breaks my syntax highlighter ¯_(ツ)_/¯
lib/universe/npc/seed.ex, line 112 at r2 (raw file):
Enum.each
Done.
lib/universe/npc/seed.ex, line 113 at r2 (raw file):
Enum.each
Done.
lib/universe/npc/internal/web.ex, line 12 at r2 (raw file):
I think this should accept an `Network.idt`
Done.
lib/universe/npc/model/npc.ex, line 9 at r2 (raw file):
I think we're using the following order: `use > import > alias`
Done.
lib/universe/npc/model/npc.ex, line 15 at r2 (raw file):
what abount `npc_type` ?
Done.
lib/universe/npc/model/npc.ex, line 52 at r2 (raw file):
alias queryable
Done.
priv/repo/network/migrations/20170712002537_dns.exs, line 10 at r2 (raw file):
This is wrong. The PK should be `{network, hostname}`, not `{hostname, network}`
network first is a bad idea, since we have very low cardinality for networks. The first filter (network_id == "::") wouldn't help at all, since it would match ~90% of the table. Using name as the first filter, though, would ensure at most one row.
Also, and because of this, network_id does not need to be an index. It's just added as primary_key because of uniqueness requirements.
We can analyze both schemas later.
priv/repo/network/migrations/20170712002537_dns.exs, line 11 at r2 (raw file):
i think this should not be nullable
Done.
priv/repo/network/migrations/20170712044302_webserver.exs, line 15 at r2 (raw file):
why is this here and not on cache ? Also, i grepped lib and it seems this table is not being used
Yep, old code
test/cache/action/cache_test.exs, line 201 at r2 (raw file):
erm what ?
Done.
test/cache/action/cache_test.exs, line 214 at r2 (raw file):
You're traversing the input to cause a side-effect (and don't care about it's return). You should not map it. Use either `Enum.each(server1.components, &(assert StatePurgeQueue.lookup(:component, &1))` or `assert Enum.all?(server1.components &StatePurgeQueue.lookup(:component, &1))`
Done.
test/cache/query/cache_test.exs, line 192 at r2 (raw file):
`describe "from_nip_get_web/2"`
Done.
test/cache/query/cache_test.exs, line 193 at r2 (raw file):
`test "returns webserver bound to nip"`
the idea behind all these it works is to represent the default/expected/happy case of the described function, but yeah, I agree it's better to be specific
test/hardware/internal/network_connection_test.exs, line 42 at r2 (raw file):
I'd recomend not assuming a hardcoded entry, instead generating a random ip and assigning it, so you can be sure that, even if the seed is changed (and it will), this test will still pass
Done.
test/hardware/internal/network_connection_test.exs, line 60 at r2 (raw file):
`test "returns the specified network connection"`
nope, phoebe is sacred
test/hardware/internal/network_connection_test.exs, line 73 at r2 (raw file):
`describe "fetch_by_nip/2"`
Done.
test/hardware/internal/network_connection_test.exs, line 74 at r2 (raw file):
`test "returns the network connection bound to specified nip"`
Done.
test/hardware/internal/network_connection_test.exs, line 81 at r2 (raw file):
`assert nc`
Done.
test/network/action/dns_test.exs, line 38 at r2 (raw file):
Add a comment to make it obvious that the reason such error is raised is because on an unicast you tried to register two different ips (and an unicast can only point to a single ip)
Done.
test/network/action/dns_test.exs, line 46 at r2 (raw file):
`test "removes the entry"` or `removes unicast entry` or `removes specified entry`
We can live with tat
test/network/internal/web_test.exs, line 12 at r2 (raw file):
Since internet is a constant, i'd prefer to use the helper (and avoid assuming that the internal accepts binary id)
Done.
test/network/query/dns_test.exs, line 28 at r2 (raw file):
why are you binding this way ? prefer two separate lines since you're not using the boxed type for anything else
Done.
test/network/query/dns_test.exs, line 41 at r2 (raw file):
lacking an assertion.
Done.
test/network/query/web_test.exs, line 11 at r2 (raw file):
`test "accepts ip"`
Done.
test/network/query/web_test.exs, line 20 at r2 (raw file):
`test "accepts hostname"`
Done.
test/network/query/web_test.exs, line 29 at r2 (raw file):
`test "fails when ip doesn't exist"`
Done.
test/network/query/web_test.exs, line 34 at r2 (raw file):
`test "fails when domain doesn't exist"`
Done.
test/server/query/server_test.exs, line 44 at r2 (raw file):
parens on function call
Done.
test/server/query/server_test.exs, line 46 at r2 (raw file):
why are you binding instead of directly asserting the function ? (ie: you're not using the binding for anything else and it doesn't breaks the column limit)
Done.
test/support/id_case.ex, line 12 at r2 (raw file):
It's not clear what this exactly does and i don't believe this belongs to this helper
Done.
test/support/id_case.ex, line 18 at r2 (raw file):
I don't think this clause is really ideal. I think `assert_id` should only accept a single id and compare it with another single id
It works and makes my life much easier, so it's fine
test/support/id_case.ex, line 21 at r2 (raw file):
spaces between each clause
?
test/support/npc_helper.ex, line 43 at r2 (raw file):
I understand the `CacheHelper.empty_cache()` but i can't understand why you're deleting everything else when we have migrations that ensure that the database contains the basic environment and we even use sandbox so the database state is always clean
Because any calls during the test setup could affect the cache. It likely will never make any difference on this case, but it doesn't hurt to be extra sure anyways
test/support/hell/setup.ex, line 16 at r2 (raw file):
remove this linebreak
Done.
test/universe/npc/internal/npc_test.exs, line 21 at r2 (raw file):
i think you should use a factory to insert an npc like on other tests
The NPCHelper is a Factory, just a badly named one.
I didn't call it Factory because it behaves differently from the other factories. We should come up with a pattern for this. Granted, I plan to use existing Internal and Query functions on those "Factories", so if this breaks your idea of Factory then it makes sense to have a different name.
test/universe/npc/internal/npc_test.exs, line 25 at r2 (raw file):
fails when npc doesn't exist
Done.
test/universe/npc/internal/npc_test.exs, line 28 at r2 (raw file):
On the other domains we are expecting `fetch` to receive an id term instead of a binary input. Use the id generation function
Done.
test/universe/npc/internal/npc_test.exs, line 32 at r2 (raw file):
i think you should use a factory to insert an npc like on other tests
Done.
test/universe/npc/internal/npc_test.exs, line 36 at r2 (raw file):
add some spaces between the phases of your test
Done.
test/universe/npc/query/npc_test.exs, line 12 at r2 (raw file):
no need for the `it` part. prefer `test "accepts npc id"`
Done.
test/universe/npc/query/npc_test.exs, line 13 at r2 (raw file):
i think you should use a factory to insert an npc like on other tests
Done.
test/universe/npc/query/npc_test.exs, line 17 at r2 (raw file):
no need for the `it` part. prefer `test "accepts entity id"`
Done.
test/universe/npc/query/npc_test.exs, line 23 at r2 (raw file):
no need for the `it` part. prefer `test "fails if npc doesn't exists"`
Done.
test/universe/npc/query/npc_test.exs, line 24 at r2 (raw file):
Why don't you just `NPC.ID.generate()` instead ?
Because I didn't know about it
Comments from Reviewable
umamaistempo
commented
7 years ago Reviewed 2 of 70 files at r2, 38 of 39 files at r3. Review status: 78 of 81 files reviewed at latest revision, 14 unresolved discussions.
lib/cache/model/cacheable.ex, line 166 at r2 (raw file):
Actually, it *does* change the data set. Just tested here. Basically, `create_changeset` will cast HELL.ID to HELL.PK (binary), which Cache uses internally (format_input is used to receive data from Helix, so this conversion happens there). In some cases such conversion does not happen, as is the case for WebCache, because if you see at builder.ex, the only function it receives as input are nip and content. Nip is already internal to cache, so it already is saved as PK.t. Content does not have an ID so nothing happens. Contrast it to, say, Storage. It adds as input storage_id and server_id. storage_id comes from cache internally, so it's already in PK.t, but server_id comes from Helix and must be casted to PK.t. WebCache is the only one who does not need to convert Helix ID to PK.t... BUT I'll leave the function there, since it's the expected behavior that whatever internal ID I throw at the Model, it will cast from ID to PK. Sample output: **before** ``` %Helix.Cache.Model.ServerCache{__meta__: #Ecto.Schema.Metadata<:built, "server_cache">, components: [%Helix.Hardware.Model.Component.ID{id: {17, 0, 0, 30882, 31197, 33120, 3786, 60482}, root: Helix.Hardware.Model.Component}, %Helix.Hardware.Model.Component.ID{id: {17, 0, 0, 33954, 42799, 15611, 26426, 54429}, root: Helix.Hardware.Model.Component}, %Helix.Hardware.Model.Component.ID{id: {17, 0, 0, 54586, 17895, 25973, 6768, 65403}, root: Helix.Hardware.Model.Component}, %Helix.Hardware.Model.Component.ID{id: {17, 0, 0, 10484, 37183, 24985, 13406, 21059}, root: Helix.Hardware.Model.Component}], entity_id: %Helix.Entity.Model.Entity.ID{id: {1, 0, 0, 17481, 37268, 64059, 21863, 2930}, root: Helix.Entity.Model.Entity}, expiration_date: nil, motherboard_id: %Helix.Hardware.Model.Component.ID{id: {17, 0, 0, 34922, 33461, 32944, 6682, 37167}, root: Helix.Hardware.Model.Component}, networks: [%{ip: "69.151.89.119", network_id: %Helix.Network.Model.Network.ID{id: {0, 0, 0, 0, 0, 0, 0, 0}, root: Helix.Network.Model.Network}}], resources: %{cpu: 1333, hdd: 1024, net: %{"::" => %{downlink: 100, uplink: 100}}, ram: 1024}, server_id: "10::2fe9:45dd:b807:57a0:a7ff", storages: [%Helix.Software.Model.Storage.ID{id: {32, 1, 0, 48322, 49514, 44211, 49644, 1372}, root: Helix.Software.Model.Storage}]} ``` **after** ``` %Helix.Cache.Model.ServerCache{__meta__: #Ecto.Schema.Metadata<:built, "server_cache">, components: ["11::78a2:79dd:8160:eca:ec42", "11::84a2:a72f:3cfb:673a:d49d", "11::d53a:45e7:6575:1a70:ff7b", "11::28f4:913f:6199:345e:5243"], entity_id: "1::4449:9194:fa3b:5567:b72", expiration_date: #DateTime<2017-08-12 06:02:07.590Z>, motherboard_id: "11::886a:82b5:80b0:1a1a:912f", networks: [%{ip: "69.151.89.119", network_id: "::"}], resources: %{cpu: 1333, hdd: 1024, net: %{"::" => %{downlink: 100, uplink: 100}}, ram: 1024}, server_id: "10::2fe9:45dd:b807:57a0:a7ff", storages: ["20:1:0:bcc2:c16a:acb3:c1ec:55c"]} ```
But the problem persists:
To create the initial ServerCache you ideally would have to execute some thing like ServerCache.create, and at that moment the values would be cast to IDs, so this protocol function is redundant and a code smell (as it is a workaround because the values are not being cast at the model and instead just "built" without any casting/validation i think)
lib/entity/query/entity.ex, line 71 at r2 (raw file):
:+1: I've changed the `value` catch-all to two explicit Entity conditions. Any other calls should crash (e.g. when we add clan).
Talked with you personally, we're gonna stick with my suggestion as it effectively works as an extension over the ID.cast :3
lib/hell/hell/ip.ex, line 145 at r3 (raw file):
|> String.to_charlist() |> :inet.parse_ipv4strict_address() end
You're using this function just to check if a certain binary ip is valid, you're using ? at the function name (which implies that it returns a boolean) but you're returning a composite type and ignoring it's result.
Fix by either renaming this by binding this return and executing match?({:ok, _}, parsed_tuple)
lib/network/model/web/player.ex, line 1 at r2 (raw file):
Because it belongs to the player, and only to the player. NPC and Clans have different handlers
OK :+1: got it
lib/network/query/web.ex, line 12 at r2 (raw file):
Nope. It's a map with the web server info, which is quite custom for npcs. Once we have a better idea of what it looks like, we can specify a better type (something like: `web_npc | web_vpc | web_clan`)
OK :+1:
lib/network/query/web.ex, line 29 at r2 (raw file):
Done.
I think you misstyped the type
lib/network/query/web.ex, line 45 at r3 (raw file):
end @spec serve(Network.id, IPv4.t) ::
I think this should receive Network.idt
lib/network/query/web.ex, line 53 at r3 (raw file):
{:ok, content} _ -> :notfound
Since you're using {:ok, term}, i'd return on error {:error, :notfound}, i think
lib/universe/npc/seed.ex, line 100 at r2 (raw file):
That breaks my syntax highlighter ¯\_(ツ)_/¯
OK. kek :joy:
priv/repo/network/migrations/20170712044302_webserver.exs, line 15 at r2 (raw file):
Yep, old code
I think we should remove it :B
test/hardware/internal/network_connection_test.exs, line 60 at r2 (raw file):
nope, phoebe is sacred
OK
test/server/query/server_test.exs, line 46 at r2 (raw file):
Done.
Not done ?
test/support/id_case.ex, line 12 at r2 (raw file):
Done.
????
Comments from Reviewable
renatomassaro
commented
7 years ago Review status: 73 of 81 files reviewed at latest revision, 10 unresolved discussions.
lib/cache/model/cacheable.ex, line 166 at r2 (raw file):
ideally would have to execute some thing like ServerCache.create, and at that moment the values would be cast to IDs
I do. ServerCache.new(). It wraps and sends to the Cacheable. The idea behind Cacheable is to be the central location where
1) IDs are handled, both from Helix to cache and from cache to Helix 2) Data is formatted, both from Helix to cache and from cache to Helix.
Hence the two implementable functions format_input and format_output. By leaving this kind of logic outside ServerCache model I can ensure that the Cache is effectively transparent to the Helix format, and the conversion only happens in one place.
the values are not being cast at the model
Not sure I got this. They are being casted with Changeset.cast/3, automatically according to the custom types ID / PK, so it is at the model level.
priv/repo/network/migrations/20170712044302_webserver.exs, line 15 at r2 (raw file):
I think we should remove it :B
I think I forgot to remove it :B
test/server/query/server_test.exs, line 46 at r2 (raw file):
Not done ?
Not done
test/support/id_case.ex, line 12 at r2 (raw file):
????
It asserts two maps are identical to each other, even if one uses ID and the other uses PK.
Example: %{network_id: "::", ip: "1.2.3.4"} == %{network_id: Network.ID{...}, ip: "1.2.3.4"}
As for belonging to the helper, "It works and makes my life much easier, so it's fine" and that's exactly what helpers are supposed to do :P
Comments from Reviewable
sourcelevel-bot[bot]
commented
7 years ago Ebert has finished reviewing this Pull Request and has found:
But beware that this branch is 11 commits behind the HackerExperience:master branch, and a review of an up to date branch would produce more accurate results.
You can see more details about this review at https://ebertapp.io/github/HackerExperience/Helix/pulls/228.
umamaistempo
commented
7 years ago Reviewed 2 of 70 files at r2, 1 of 39 files at r3, 5 of 5 files at r4, 7 of 7 files at r5. Review status: all files reviewed at latest revision, all discussions resolved.
Comments from Reviewable
Note: This PR depends on #227 (edit: and #231) being merged first. Once it's merged, it will produce the actual diff. I don't think I can open this PR based on another PR (and I don't want to open it against my own repository).This PR introduces:
Missing:
Out of scope:
Overall, this was a great 10-hour marathon. I've missed this. If we keep this pace, HE2 is out in 5 days. #PlanoJK50DiasEm5
This change is